net/http: possibly unaligned 64-bit word accessed atomically

[pam4]

Here it is.

If I understand the alignment problem correctly, a struct nested in another struct is not by itself "allocated", therefore you cannot assume its first word is 64-bit aligned.

However there is a fair chance that I'm missing something, see the CL. It seems it was done quite on purpose.

If I'm misunderstanding the alignment problem, please close this and I'll file a bug for the atomic documentation because the "allocated" wording in the Bugs section is very confusing.

Notice that sync.WaitGroup goes to all the trouble of making an aligned 64-bit word out of a [3]uint32 using unsafe.Pointer, here.

Another suspicious 64-bit word accessed atomically is field numClosed in the DB struct of database/sql. I'm not sure it's safe to assume that its preceding field size is a multiple of 64 bit (interface type).

I'm concerned about other cases too. For example I'm not familiar with the expvar package, but if a user makes a struct with a field of type expvar.Int (not a pointer), he's going to cause trouble. Is it obvious that you can't do that?

[ianlancetaylor]

The code in net/http works because the field is comes after an even number of pointers. Same for DB.NumClosed. This code is certainly fragile and there ought to be a better way. One proposal is #19057 but it hasn't gone anywhere. There is also #599.

I'm going to close this issue since there isn't a bug here.

[pam4]

@ianlancetaylor, sorry I don't get it.

• In the CL you commented "I don't see anything keeping this aligned to a 64-bit boundary on a 32-bit system", and the field was already at the same exact position.
  Then @bradfitz changed it from type uint64 to type struct{ atomic uint64 } and it was fixed. What's the meaning of that change?

• How do you count the even number of pointers?
  Before conn.curState I see 5 pointers, 2 interfaces, 2 strings, 1 func(), and 1 struct from an external package.
  I assume the func() count as 1 pointer and interfaces and strings count as 2 pointers for a total of 14 pointers.
  Then there is the atomic.Value which is struct {v interface{}} -> 2 more pointers and we are still even.
  Seriously? This is what I have to do to see that this code doesn't crash and there's not even a comment in it?
  And it could even be affected by a change in another package.

I'm aware of the issues you mentioned. I doubt there's something I haven't read already on the subject, but it's still a big confusion.
What about expvar.Int and similar? If the zero value is useful, a user may want to put it in his own struct, unaware of the alignment requirement.

[ianlancetaylor]

I think that I wasn't thinking when looking at that CL. I agree that I don't see why Brad's change helps the problem.

We have a general problem with atomic access to 64-bit words on 32-bit systems, and that needs a general solution. I encourage you to open an issue about that, especially if it is not already covered by the issues I mentioned above. I'm not sure it helps to open an issue about a specific problem in net/http.

[pam4]

@ianlancetaylor, thanks for the clarification.

I can live with the general problem, I'm more concerned about the "clarity" problem.
The code (and the CL) made me doubt my own understanding (which was actually correct) and may confuse other people as well.

I expected to find 64-bit fields accessed atomically, only at the top of their own struct and well marked; I never suspected a pointer count on 11 fileds was involved.

I'm not sure it helps to open an issue about a specific problem in net/http.

Well, while we are at it, I think moving curState on top of the http.conn struct, with a nice comment like // keep this field 64-bit aligned, and removing the misleading nested struct would help a lot.

It seems far too likely that the next contributor changing http.conn layout will miscalculate or even overlook the whole problem.

EDIT: if http.conn zero-values are not supposed to be ready to use, we can do even better: make curState a *uint64 and initialize it using new, so that the field position is irrelevant.