google / google/gopacket

`(*PacketSource).packetsToChannel()` error handling is incomplete

Open
#1,143 1 comment 1 reaction 0 assignees View on GitHub
Dominant language
Go
Stars
6.8k
Forks
1.2k
PR merge metrics
No merged PRs in 30d

Description

## TL;DR
The error handling in `(*PacketSource).packetsToChannel()` is incomplete and may lead to starvation. A catch-all rule is missing.

## Minimal Example

### Implementation
```go
f, _ := os.Open("../testdata/somepackets.pcap")
defer f.Close()
r, _ := pcapgo.NewReader(f)

pktsrc := gopacket.NewPacketSource(r, r.LinkType())

f.Close() // close the reader here

for packet := range pktsrc.Packets() { // this will block after the last packets
// do something
}
```
### Behavior
The program hangs instead of terminating.

## Analysis

https://github.com/google/gopacket/blob/32ee38206866f44a74a6033ec26aeeb474506804/packet.go#L815-L845

The function implementation missed the case where the returned error is not matched to any known cases.

In the current latest version of Go (1.21.4), `(*os.File).Read()` returns `read /path/to/the/file: file already closed`, which wraps `os.ErrClosed` and could be tested with `errors.Is(err, os.ErrClosed)`, but is not __equal__ to any existing error. Therefore it will not break out of the `for` loop and the channel will never be closed.

## Proposed fix
- Testing against known errors should use `errors.Is` or its equivalent, not `err == ErrToCompare`.
- Should also add a catch-all case for unknown errors, since it is possible for the user to use a custom `io.Reader`.

I'd be happy to open a PR if the proposed fix sounds reasonable.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.