Articles / Go

702 words

  • go
  • hashing
  • io-reader
  • buffers
  • sha256
  • debugging

Your Go Hash Is Wrong Because You Hashed the Whole Buffer

Here is a checksum function that passes every test you are likely to write and then returns the wrong digest in production. It reads a file in chunks, feeds a hash, and gets the last chunk subtly wrong. The wrongness only shows up once the buffer has been used for something else first.

func hashFile(path string, buf []byte) (string, error) {
	f, err := os.Open(path)
	if err != nil {
		return "", err
	}
	defer f.Close()

	h := sha256.New()
	for {
		n, err := f.Read(buf)
		h.Write(buf) // bug: should be buf[:n]
		if err == io.EOF {
			break
		}
		if err != nil {
			return "", err
		}
	}
	return hex.EncodeToString(h.Sum(nil)), nil
}

What it actually hashes

Read fills up to len(buf) bytes and tells you how many through n. The rest of the slice is left alone. Passing buf to Write hashes the stale tail as if it were file content.

Take a 10 byte file and a 32 KiB buffer. The hash covers 10 real bytes plus 32,758 bytes of whatever was there before. If the buffer is freshly allocated that is zeros, so you get a consistent digest that matches nothing else on earth. It looks like a hash. It is just not the hash of your file.

Why the tests stay green

With a fresh make([]byte, 32*1024) per call, the output is deterministic. Hash the same file twice and you get the same answer, so a "same input gives same output" test passes happily.

The failure appears when the buffer comes from a sync.Pool, or a struct field, or a loop that hoists the allocation. Now the stale tail is the end of the previous file, and the same file hashes differently depending on what the worker did a moment ago. That reads like a data race, but there is no concurrency involved at all.

The fix is one slice expression

n, err := f.Read(buf)
h.Write(buf[:n])

Two more rules belong with it, both straight from the io.Reader documentation:

  • Process the n bytes before looking at err.
  • Treat io.EOF as a normal end, and anything else as a failure.
  • Never assume a short read means the end; only the error says that.

The first rule matters because a reader is allowed to return n > 0 together with io.EOF. An os.File usually returns 0, io.EOF on the following call, so the order looks harmless. Plenty of other readers do hand back the last bytes with the error, and a loop that checks err first silently drops them.

Or skip the loop entirely

A hash is an io.Writer, so the standard library already has the loop, with the slicing and the error ordering done correctly.

h := sha256.New()
if _, err := io.Copy(h, f); err != nil {
	return "", err
}
sum := h.Sum(nil)

Unless you need a specific buffer size for a reason you can name, this is the version to write. If you do need one, io.CopyBuffer(h, f, buf) takes your buffer and still does the right thing with n.

Quick detour: the same bug with a channel

Hang on, there is a nastier cousin. A reader goroutine fills a buffer and sends it down a channel to a hashing goroutine, then loops round and reads into the same buffer again.

for {
	n, err := f.Read(buf)
	if n > 0 {
		ch <- buf[:n] // receiver gets a view, not a copy
	}
	if err != nil {
		break
	}
}

Sending a slice sends a pointer to the same backing array. The receiver may be mid-hash when the next Read overwrites the bytes. Now the digest depends on scheduling, and go test -race will flag it. Either copy the chunk before sending, or pass ownership: allocate a fresh slice per chunk, or hand buffers back on a second channel so nobody writes to one that is still in use.

Catching it with a test

The cheap trick is to make stale data impossible to miss. Fill the buffer with a non-zero pattern before the call, and use readers that misbehave in the ways the interface permits.

func TestHashIgnoresStaleBuffer(t *testing.T) {
	data := []byte("hello, world")
	want := sha256.Sum256(data)

	buf := bytes.Repeat([]byte{0xff}, 64)
	h := sha256.New()
	r := iotest.DataErrReader(iotest.OneByteReader(bytes.NewReader(data)))
	if err := hashReader(h, r, buf); err != nil {
		t.Fatal(err)
	}
	if !bytes.Equal(h.Sum(nil), want[:]) {
		t.Fatal("digest depends on buffer contents or read pattern")
	}
}

iotest.OneByteReader forces tiny reads, so n is almost never len(buf). iotest.DataErrReader returns the final bytes together with io.EOF. Together with the 0xff fill, the three mistakes above (hashing buf, checking err first, assuming full reads) all fail loudly. Here hashReader is whatever your function that takes a hash, a reader and a buffer is called.

The general rule

A buffer you pass to Read is scratch space, and only buf[:n] means anything afterwards. The same applies to bufio.Scanner.Bytes() and bufio.Reader.Peek: the returned slice is valid until the next call, so copy it if you keep it. A stale tail that happens to be zeros just hides the bug until the day it is not.

← All articles