Skip to content

Conversation

@glaslos
Copy link
Member

@glaslos glaslos commented May 24, 2025

  • Refactored storing data on disc through a single helper
  • Ticker to clean up old connections in the tracker

@glaslos glaslos requested a review from Copilot May 24, 2025 14:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors how binary data is stored and embeds default configuration data while also introducing a periodic cleanup of stale connections.

  • Refactored helper function usage for storing payloads across protocols.
  • Embedded a default configuration file in Glutton and updated connection management with a ticker-based flush.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
protocols/udp/udp.go Updated function call to use unified Store helper with "payloads" folder.
protocols/tcp/telnet.go Refactored sample fetching to use the new Store helper and improved error messaging.
protocols/tcp/tcp.go Replaced deprecated StorePayload with new Store helper.
protocols/helpers/helpers.go Renamed StorePayload to Store and generalized folder handling.
glutton.go Embedded default configuration and modified initConfig logic.
connection/connection_test.go Updated tests to pass context when creating connection table.
connection/connection.go Modified New() to accept context and added a periodic flush ticker.
app/server.go Updated SSH port default from 0 to 22.

@glaslos
Copy link
Member Author

glaslos commented May 24, 2025

@namay26 and @Boolean-Autocrat can you give this a code review?

@glaslos glaslos requested a review from Copilot May 24, 2025 14:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors payload storage into a unified helper and adjusts configuration loading by embedding a default configuration. Key changes include:

  • Changing payload storage from StorePayload to a more flexible Store function.
  • Refactoring the Telnet sample retrieval with improved error messages and inline error handling.
  • Updating connection management to use context for periodic cleanup and embedding a default configuration in glutton.go.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
protocols/udp/udp.go Updated payload logging and storage using a min-based slicing logic.
protocols/tcp/telnet.go Improved error messages, URL trimming, and inline error handling in sample retrieval.
protocols/tcp/tcp.go Updated payload storing API call to use the new Store function.
protocols/helpers/helpers.go Renamed StorePayload to Store with added folder flexibility.
glutton.go Embedded a default configuration and updated connection table usage.
connection/connection.go Modified New to accept context and added a ticker for periodic cleanup.
connection/connection_test.go Updated tests to pass context when creating connection tables.
app/server.go Revised default SSH port flag.
Comments suppressed due to low confidence (2)

connection/connection.go:65

  • Ensure that the context provided to connection.New is appropriately canceled when no longer needed to prevent potential goroutine leaks from the ticker loop.
go func() { ... for { select { case <-ctx.Done(): return ... } } }()

protocols/udp/udp.go:16

  • Ensure that the helper function 'min' is defined in the current scope and correctly returns the minimum of two int values. If not, consider adding a simple implementation or utility function to replace its usage.
log.Info(fmt.Sprintf("UDP payload:\n%s", hex.Dump(data[:min(len(data), 1024)])))

@glaslos glaslos requested a review from Boolean-Autocrat May 24, 2025 14:52
@Boolean-Autocrat
Copy link
Collaborator

Boolean-Autocrat commented May 26, 2025

Hi! So this mostly looks good; embedding the default config is a nice approach to provide a fallback.

I have a small question/concern in the way the context lifecycle is working in ConnTable. The cleanup goroutine in connection.New() stops when the passed context is canceled during Glutton.Shutdown(), but the ConnTable itself remains alive and continues accepting new connections while listeners are shutting down. Since these potential "post-cancellation" connections are not cleaned up, it could be a memory leak.

For now, in Glutton the process exits immediately after shutdown but if (maybe in the future) we decide to introduce lifecycle changes (like a restart without process exit), it will probably create issues. We could add a g.connTable.FlushOlderThan(0) in Shutdown() or maybe give ConnTable its own lifecycle. I honestly might be thinking a bit too much about this subtle detail and it might just be intended behavior; keeping it the same is cool too.

@namay26
Copy link
Collaborator

namay26 commented May 27, 2025

I agree with the analysis by @Boolean-Autocrat. If we do decide on adding the Restart process, the current setup would create memory leaks.
Rest assured, LGTM.

@glaslos
Copy link
Member Author

glaslos commented May 30, 2025

@Boolean-Autocrat that is an interesting edge case. We can address this when we introduce a restart feature. I would assume during restart you wouldn't cancel the main context.

@glaslos glaslos merged commit f9958de into main May 30, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants