Bug Description

In file: app.go, the function Test starts a goroutine that serves a connection using app.server.ServeConn(conn) and handles errors using a channel. The return value of ServeConn is sent to channel and also a defer function sends ErrHandlerExited to channel if the goroutine exits unexpectedly. The main function receives from the channel using a select statement. The select statement has a separate case for closing the connection when cfg.Timeout duration has elapsed.

// Serve conn to server
channel := make(chan error)
go func() {
	var returned bool
	defer func() {
		if !returned {
			channel <- ErrHandlerExited
		}
	}()

	channel <- app.server.ServeConn(conn)
	returned = true
}()

// Wait for callback
if cfg.Timeout > 0 {
	// With timeout
	select {
	case err = <-channel:
	case <-time.After(cfg.Timeout):
		conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here
		if cfg.FailOnTimeout {
			return nil, os.ErrDeadlineExceeded
		}
	}
} else {
	// Without timeout
	err = <-channel
}

The receive case in the select statement doesn't execute if timeout occurs first. In this case, any sending operation in the goroutine will block indefinitely since the channel is unbuffered. Such cases will lead to unnecessary CPU and memory usage when each invocation of the function Test spawns a separate goroutine.

We are filing this issue because it is theoretically a bug. However, this is in the Test function. So this may be a bug that the maintainers may want to ignore (there is also a comment but it is unclear which bug it is choosing to ignore). But we cannot make that assessment. Moreover, the fix is probably easy. So we are filing this bug.

How to Reproduce

The bug was detected using static analysis as there is the possibility of channel blocking in cases where timeout occurs first. So we did not reproduce the bug by executing source code.

Expected Behavior

The bug was detected using static analysis, as there is the possibility of channel blocking in cases where timeout occurs first. In this case, rather than expected, appropriate might be a better word. Here more appropriate behavior would be every goroutine finishing execution without waiting indefinitely.

Fiber Version

2.52.6

Possible Fix

One possible fix could be making the error channel buffered with capacity 1. In this way sending operations in the started goroutine can complete without blocking even if the main routine doesn't receive the sent value in some cases.

// Serve conn to server
channel := make(chan error, 1)
go func() {
	var returned booi
	defer func() {
		if !returned {
			channel <- ErrHandlerExited
		}
	}()

	channel <- app.server.ServeConn(conn)
	returned = true
}()

// Wait for callback
if cfg.Timeout > 0 {
	// With timeout
	select {
	case err = <-channel:
	case <-time.After(cfg.Timeout):
		conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here
		if cfg.FailOnTimeout {
			return nil, os.ErrDeadlineExceeded
		}
	}
} else {
	// Without timeout
	err = <-channel
}

Sponsorship and Support:

Sponsorship and Support:
This work is done by the security researchers from OpenRefactory under the Project Clean Beach initiative. The researchers are proactively scanning critical open source projects and finding previously undetected bugs in them. The bug is found by running the iCR tool by OpenRefactory and then manually triaging the results.