Skip to content

Fix security vulnerabilities, race conditions, panics, and logic bugs#2

Draft
Copilot wants to merge 6 commits into
masterfrom
copilot/analyze-security-and-logic-issues
Draft

Fix security vulnerabilities, race conditions, panics, and logic bugs#2
Copilot wants to merge 6 commits into
masterfrom
copilot/analyze-security-and-logic-issues

Conversation

Copy link
Copy Markdown

Copilot AI commented Nov 28, 2025

Analysis of the codebase revealed multiple security and reliability issues including race conditions, nil pointer dereferences, resource leaks, unbounded recursion, and incorrect concurrency patterns.

Fixes

Concurrency Issues

  • Race condition in cache logging (telegram/cache.go): Accessed map sizes without holding lock
  • Race condition in album handlers (telegram/updates.go): Iterated dispatcher handlers without lock; now deep copies under RLock
  • Stop channel race (telegram/client.go): Added mutex-protected resetStopChannel() to prevent goroutine leaks

Nil Pointer Dereferences

  • TCP read error handling (internal/transport/connection.go): Fixed logic that could deref nil e.Err when checking for io.ErrClosedPipe
  • Sender cleanup (telegram/client.go): Added nil check before accessing sender.GetLastUsedTime()

Resource Leaks

  • File descriptor leak (mtproto.go): os.OpenFile was never closed during permission check

Panic Prevention

  • CloseChannelWithoutPanic (internal/utils/sync.go): Function never actually closed channel; now closes with specific panic recovery
  • Error parsing panic (errors.go): Replaced panic() with log on unexpected reflect.Kind

Logic Bugs

  • Defer inside loop (telegram/updates.go): Removed defer in loop (defer runs on function exit, not iteration end)
  • Notification timeout (mtproto.go): Increased from 1ms to 100ms to prevent dropped messages
  • Unbounded recursion (handshake.go): Auth key generation could recurse infinitely; added 3-retry limit

Example Fix

// Before: only closed on panic
func CloseChannelWithoutPanic(c chan tl.Object) {
    defer func() {
        if r := recover(); r != nil {
            close(c)  // Bug: closes in panic handler, never in normal case
        }
    }()
}

// After: always closes, recovers from already-closed panic
func CloseChannelWithoutPanic(c chan tl.Object) {
    if c == nil {
        return
    }
    defer func() {
        if r := recover(); r != nil {
            if str, ok := r.(string); ok && str == "close of closed channel" {
                return
            }
            panic(r)
        }
    }()
    close(c)
}

CodeQL scan shows no security alerts.

Original prompt

Analysie for security and other issues, deadlocks, other panics, logic issues


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 5 commits November 28, 2025 13:40
- Fix CloseChannelWithoutPanic to actually close channel instead of only on panic
- Replace panic with log in TryExpandError for unexpected reflect.Kind
- Fix file descriptor leak in NewMTProto by closing file after permission check
- Increase notification timeout from 1ms to 100ms to prevent dropped messages
- Remove defer inside loop in handleRawUpdate (defer only runs on function exit)
- Fix data race in UpdatePeersToCache by holding RLock when reading cache sizes
- Fix race condition in albumBox.WaitAndTrigger by copying handlers map under lock
- Fix nil pointer dereference in tcpConn.Read for io.ErrClosedPipe handling
- Fix nil pointer dereference in cleanupIdleSenders by checking sender != nil
- Add retry limit to auth key generation to prevent unbounded recursion
- Improve stop channel handling in Start/St to prevent goroutine leaks

Co-authored-by: Praveenkumar801 <131750384+Praveenkumar801@users.noreply.github.com>
- Fix error variable usage in permission check error message
- Remove redundant makeAuthKeyInternal function

Co-authored-by: Praveenkumar801 <131750384+Praveenkumar801@users.noreply.github.com>
- Add nil check to CloseChannelWithoutPanic to prevent panic on nil channel
- Add mutex-protected resetStopChannel helper for thread-safe channel management
- Deep copy handler slices in WaitAndTrigger to prevent race conditions

Co-authored-by: Praveenkumar801 <131750384+Praveenkumar801@users.noreply.github.com>
Only recover from "close of closed channel" panics, re-panic for other
unexpected panic types to avoid masking programming errors.

Co-authored-by: Praveenkumar801 <131750384+Praveenkumar801@users.noreply.github.com>
Only recover from exact "close of closed channel" panic message to avoid
suppressing other legitimate panics.

Co-authored-by: Praveenkumar801 <131750384+Praveenkumar801@users.noreply.github.com>
Copilot AI changed the title [WIP] Analyze for security issues, deadlocks, and logic errors Fix security vulnerabilities, race conditions, panics, and logic bugs Nov 28, 2025
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.

2 participants