Doing it the hard way

In my last post I offered to point out some things in Golang Challenge #2 submissions that struck me as “worthy of receiving a (polite) rebuke in code review”, otherwise known as WTFs.

This is opt-in abuse. I don’t mind abusing my colleagues, when I know I can take them out for lunch later and buy them a beer. Hassling random Golang Challenge entrants is not my style. But some have decided they are up for it, even if I’m remote and can’t buy them a beer afterwards.

io.Toys

First up, is Jason Clark. Jason’s entry is quite good, from an idiomatic Go point of view. He fell prey to the “under-testing” problem I mentioned, in that his entry does not handle message framing, or what happens if the buffer passed to Read is smaller than the incoming message. But what I’m really looking for for the purposes of this post are the WTF’s. And seeing io.TeeReader tipped me off that something interesting was about to happen:

// Write encrypts the contents of p and writes it to the underlying io.Writer.
func (w *SecureWriter) Write(p []byte) (int, error) {
	// Each message starts with a generated nonce. Only write the nonce once.
	if w.nonce == nil {
		var nonce [24]byte
		if _, err := io.ReadFull(io.TeeReader(rand.Reader, w.w), nonce[:]); err != nil {
			return 0, ErrNonceWrite
		}
		w.nonce = &nonce
	}

	// encrypted and send message
	_, err := w.w.Write(box.SealAfterPrecomputation(nil, p, w.nonce, w.key))
	if err != nil {
		return 0, err
	}
	return len(p), nil
}

So, the first problem here is that this is just wrong. The docs for NaCl are clear that security is compromised if you use the same nonce for two different messages. But second, the use of io.TeeReader here is overly clever. I didn’t even know what io.TeeReader did, but the docs are clear enough: “TeeReader returns a Reader that writes to w what it reads from r”. So what that line does is get the nonce from cyrpto.Rand, and send the nonce down the unencrypted channel (correct!) while also sending it into nonce, to be saved for later as w.nonce (not correct!). To reason about that line as a reader you need to keep two unrelated processes in mind at once (saving the nonce for later, and sending it down now), and then ask yourself if the error handling correct in both of those cases.

So, if ErrNonceWrite is returned, was a nonce sent down the connection anyway? What was it?

The simpler way of doing this is in two steps. First read into a local called nonce, and check the errors on reading. Once you are sure you have a good nonce, send it down with w.Write(). Checking errors correctly here is critical, because if you don’t manage to get a new nonce, you must not send the message, since you might be sending it with a nonce of all zeros. The second time you’ve done this, you’ve compromised your security.

There’s nothing wrong with using the nifty toys in the io package. And in fact, later in his submission, Jason uses io.MultiWriter in an interesting way. I can’t say that I’m personally 100% confident in the technique of using io.Copy to implement an echo server. I understand the theory, but it seems to me you become dependent on the right magic happening inside of io.Copy, instead of making sure that the logic of your program is front and center, where your reader can see it.

Always remember, code is for reading. When you work on a big system, with lots of coworkers, they will read and re-read your code for months and years after you’ve written it. Code is written once and read a thousand times. Make sure what’s there to read explains what you are doing and what you are depending on.

And now for a random global problem…

Trevor Arjeski got in touch and asked if he had any WTFs. By chance, the first file I clicked on was secure_io.go, and within the first few lines I knew, “we have a winner!”

In a crypto system, the quality of your random numbers matters. This has been known by professional cryptographers for decades, and to the rest of us since 1996. So when I saw that Trevor was importing “math/rand” instead of “crypto/rand”, I knew that he had a big problem coming.

Worse, I read down a few lines and saw the global “seeded”. The problem with this global is that access to it is not protected from a data race. There are three options for how to fix this:

• Nuke it: This problem calls for using the “crypto/rand” package, which does not need seeding by the Go programmer.
• Move it to init: Lazy initialization, controlled by a global flag can sometimes be more simply moved to explicit initialization, before main starts running. Go guarantees that init functions are run in a single-threaded context (the spec says: “Package initialization—variable initialization and the invocation of init functions—happens in a single goroutine, sequentially, one package at a time.”)
• Use sync.Once to arrange for something to be lazily initialized once and only once.

I’ve said before that good testing matters. But to catch this data race, Trevor should have written a test that concurrently created multiple SecureWriters and written into them. Then he would have had to run “go test -race”. It is wonderful that Go has the race detector, but it is better to never write data races to begin with. And a good way to not write them is to not allocate any shared data.

Get in the habit of asking hard questions about every global you see: when it is initialized? Are there writes to it? How are they sequenced with respect to reads?

Want some practice? Move on to lines 14 and 15… 🙂

Addicted to encoding/binary?

Kristoffer Berdal got in touch and asked me to take a look. Kristoffer made a choice for readability that would eventually be a performance problem, if he wanted to benchmark and tune his implementation.

In his Writer we see a nice symmetrical list of calls to binary.Write. Reading through it, it makes the exact format of the protocol completely clear. So score points for self-documenting code!

But there’s a cost: check out the signature of binary.Write: func Write(w io.Writer, order ByteOrder, data interface{}) error

That means that each time we call into binary.Write, we’ll be doing a type conversion from the fundamental type of the third argument into an interface{}. This may involve an allocation, and once control gets inside of binary.Write, it will end up in the “fall back onto reflection” code path, because the nonce and (usually) the message are not less than 8 bytes long. Reflection is powerful and magical, and a strong point of Go. But it is not fast, when the alternative is not using reflection.

A better choice for writing the nonce and the message, which are simple []byte types, would have been to use the Write method of the underlying io.Writer. (Also, this saves the reader of puzzling over what the meaning of the little endian representation of a [24]byte is…)

One thing that’s interesting about Go is that it can be highly friendly and easy like Python, or highly precise and speedy like C. Choosing when to do one or the other (or finding a way to do both) is the trick that requires experience. It is always important to first get it right, then get it clear, then measure, and only if measurement says to do so, make it fast. In a case like this, Kristoffer has written it right, and clearly. But by turning to a function that uses interface{} where there was another option, he left some performance on the table. Worse, he might have left behind a trap that a colleague will need to deal with later when the prototype goes to production and starts handling gigbits of data per second.

Thanks

Thanks to those who gave me permission to pick on them. I hope that they’ve enjoyed the 15 minutes of fame, and that we’ve all learned something useful.