Fix critical bugs and security vulnerabilities
- Fix race condition in client cleanup by serializing operations - Add proper nil checks in SendMessage for server/config - Add semaphore to limit concurrent health check goroutines - Reduce buffer size to RFC-compliant 512 bytes (was 4096) - Add comprehensive input validation (length, null bytes, UTF-8) - Improve SSL error handling with graceful degradation - Replace unsafe conn.Close() with proper cleanup() calls - Prevent goroutine leaks and memory exhaustion attacks - Enhanced logging and error recovery throughout These fixes address the freezing issues and improve overall server stability, security, and RFC compliance.
This commit is contained in:
69
client.go
69
client.go
@@ -177,12 +177,22 @@ func (c *Client) SendMessage(message string) {
|
||||
c.mu.Lock()
|
||||
defer c.mu.Unlock()
|
||||
|
||||
// Enhanced connection health check
|
||||
// Enhanced connection and server health check
|
||||
if c.conn == nil {
|
||||
log.Printf("SendMessage: connection is nil for client %s", c.Nick())
|
||||
return
|
||||
}
|
||||
|
||||
if c.server == nil {
|
||||
log.Printf("SendMessage: server is nil for client %s", c.Nick())
|
||||
return
|
||||
}
|
||||
|
||||
if c.server.config == nil {
|
||||
log.Printf("SendMessage: server config is nil for client %s", c.Nick())
|
||||
return
|
||||
}
|
||||
|
||||
// Validate message before sending
|
||||
if message == "" {
|
||||
return
|
||||
@@ -958,8 +968,8 @@ func (c *Client) Handle() {
|
||||
|
||||
scanner := bufio.NewScanner(c.conn)
|
||||
|
||||
// Set maximum line length to prevent memory exhaustion
|
||||
const maxLineLength = 4096
|
||||
// Set maximum line length per IRC RFC (512 bytes including CRLF)
|
||||
const maxLineLength = 512
|
||||
scanner.Buffer(make([]byte, maxLineLength), maxLineLength)
|
||||
|
||||
// Set initial read deadline - be more generous during connection setup
|
||||
@@ -1035,38 +1045,37 @@ func (c *Client) cleanup() {
|
||||
c.mu.Unlock()
|
||||
}
|
||||
|
||||
// Part all channels with error handling in a separate goroutine to prevent blocking
|
||||
go func() {
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
log.Printf("Panic during channel cleanup for %s: %v", c.getClientInfo(), r)
|
||||
}
|
||||
}()
|
||||
|
||||
channels := c.GetChannels()
|
||||
for channelName, channel := range channels {
|
||||
if channel != nil {
|
||||
channel.RemoveClient(c)
|
||||
// Clean up empty channels
|
||||
if len(channel.GetClients()) == 0 && c.server != nil {
|
||||
c.server.RemoveChannel(channelName)
|
||||
}
|
||||
}
|
||||
// Perform cleanup operations sequentially to avoid race conditions
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
log.Printf("Panic during cleanup for %s: %v", c.getClientInfo(), r)
|
||||
}
|
||||
}()
|
||||
|
||||
// Remove from server in a separate goroutine to prevent deadlock
|
||||
go func() {
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
log.Printf("Panic during server cleanup for %s: %v", c.getClientInfo(), r)
|
||||
// Get channels snapshot before cleanup
|
||||
channels := c.GetChannels()
|
||||
var emptyChannels []string
|
||||
|
||||
// Remove client from all channels first
|
||||
for channelName, channel := range channels {
|
||||
if channel != nil {
|
||||
channel.RemoveClient(c)
|
||||
// Track empty channels for later cleanup
|
||||
if len(channel.GetClients()) == 0 {
|
||||
emptyChannels = append(emptyChannels, channelName)
|
||||
}
|
||||
}()
|
||||
|
||||
if c.server != nil {
|
||||
c.server.RemoveClient(c)
|
||||
}
|
||||
}()
|
||||
}
|
||||
|
||||
// Remove from server (must happen after channel cleanup)
|
||||
if c.server != nil {
|
||||
c.server.RemoveClient(c)
|
||||
|
||||
// Clean up empty channels after client removal to prevent race conditions
|
||||
for _, channelName := range emptyChannels {
|
||||
c.server.RemoveChannel(channelName)
|
||||
}
|
||||
}
|
||||
|
||||
log.Printf("Cleanup completed for client %s", c.getClientInfo())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user