Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling is broken for short reads in NextMessage #22

Open
juhaszp-uhu opened this issue Mar 28, 2024 · 1 comment
Open

Error handling is broken for short reads in NextMessage #22

juhaszp-uhu opened this issue Mar 28, 2024 · 1 comment

Comments

@juhaszp-uhu
Copy link

The NextMessage function is not equipped to handle short reads, it just assumes that the entire payload can be read with one syscall, and what's worse, it has a subtle bug when it attempts to report this error case.

The issue can be demonstrated with the following strace extract:

/tmp/strace.out.94:16:42:00.052698 read(7, "\20\1@", 3)    = 3
/tmp/strace.out.94:16:42:00.052804 read(7, "\10\0\370\377\370\377\10\0\370\377\370\377\370\377\370\377\370\377\10\0\370\377\10\0\370\377\10\0\370\377\370\377"..., 320) = 103
/tmp/strace.out.94:16:42:00.053075 read(7, 0xc00041abe9, 3) = -1 EAGAIN (Resource temporarily unavailable)
/tmp/strace.out.94:16:42:00.094725 read(7, "\377\370\377", 3) = 3
/tmp/strace.out.94:16:42:00.095028 read(7, "\10\0\370\377\10\0\370\377\370\377\370\377\370\377\370\377\370\377\370\377\370\377\370\377\370\377\370\377\370\377\370\377"..., 63743) = 860
/tmp/strace.out.94:16:42:00.095367 read(7, 0xc00041abf8, 3) = -1 EAGAIN (Resource temporarily unavailable)

The go code that produced this trace is very similar to the example code around here, it calls NextMessage in a loop, checks for errors, and then attempts to process the received packet in a switch, depending on the packet's Kind().
In this case it first reads a proper packet header, learns that the payload should be 320 bytes long and then attempts to read that many bytes. However, it only reads 103 bytes, and then it returns. The next call to NextMessage then interprets whatever audio sample that was in the buffer as the header of the next packet.

What makes this worse is a subtle bug in errors.Wrapf, as used in NextMessage:

	if n != int(payloadLen) {
		return nil, errors.Wrapf(err, "read wrong number of bytes (%d) for payload", n)
	}

If err is nil, as in this case, the value returned by errors.Wrapf is also nil! This can be examined in the following playground link: https://go.dev/play/p/5zHdS1_Crkr

Proposed steps to fix:

  • handle short reads properly, by calling Read() in a loop until error or enough bytes were read
  • don't use errors.Wrapf when err is nil, or even better, don't use github.com/pkg/errors at all, it's considered deprecated nowadays
@pjuhasz
Copy link

pjuhasz commented Mar 28, 2024

The fix may be as simple as using io.ReadFull instead of Read.

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

No branches or pull requests

2 participants