1130822 - casting pointers to different alignement-requiring sizes and dereferencing the result may crash

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Martin Husemann]

Attachment: memcpy the value into a temporary (with proper alignment) and handle endianes later

In Http2Session.cpp various uses of the shema

  uint32_t = PR_ntohl(*reinterpret_cast<uint32_t*>(...))

make firefox crash on alignment critical architectures

Comment 1

[Martin Husemann]

A helper function like:

     uint32_t PR_be32dec(const void *stream);

would clean this up quite a bit.

Comment 2

[Landry Breuil (:gaston)]

Comment on attachment 8560973 [details] [diff] [review]
memcpy the value into a temporary (with proper alignment) and handle endianes later

Testing this on OpenBSD/sparc64 using m-c to see if it resolves the xpcshell startup compilation crash during packaging.

Comment 3

[Daniel Stenberg [:bagder]]

Seeing the patch, this is somewhat of a pattern in this code. Can we introduce a helper-function or something that avoids the feeling of a work-around on these 11 places in the code? Perhaps that could also help us avoid re-introducing this problem again in the future.

Comment 4

[u408661]

Comment on attachment 8560973 [details] [diff] [review]
memcpy the value into a temporary (with proper alignment) and handle endianes later

Yeah, this is something that, when we take a patch for it, should probably be more generalized. Who knows, we *may* even have a function to do this in an alignment-safe way, already, somewhere in the tree. (Then again, we may not, given that all architectures that don't like unaligned access are tier 3 platforms.)

Martin (or :gaston), would you be willing to clean this up/generalize it by  putting some inline utility functions in nsNetUtil.h, and using those in Http2Session.cpp? I'll gladly review that patch. Otherwise, I can write it myself, but it will most likely take *way* longer to get done, with what I have on my plate :)

Comment 5

[Martin Husemann]

Sure, might take a few days though.

Comment 6

[Martin Husemann]

Attachment: Implement helper functions to decode arbitrary aligned data in network byte order to host endianess

Comment 7

[u408661]

Comment on attachment 8564032 [details] [diff] [review]
Implement helper functions to decode arbitrary aligned data in network byte order to host endianess

Review of attachment 8564032 [details] [diff] [review]:
-----------------------------------------------------------------

Argh! Sorry for losing track of this. This looks good, modulo the one change below. I can go ahead and make that change, and then get the patch ready for landing (including a try run, just for safety's sake).

Michal, no need for you to look at this, sorry for the bugspam :)

::: netwerk/base/nsNetUtil.h
@@ +2771,5 @@
> +inline uint16_t NS_decodeN16(const void *bytes)
> +{
> +    uint16_t tmp;
> +
> +    memcpy(&tmp, bytes, sizeof tmp);

sizeof(tmp), please (same below)

Comment 8

[u408661]

I actually moved the decoders into Http2Session.cpp, for parity with CopyAsNetwork{16,32}, which we already have. It's quite possible this pattern exists elsewhere in gecko, and we should fix it on a more global basis, but that's fodder for another bug :)

Here's a try run hitting the h2 code, assuming this passes, I'll get the patch landed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8850bc1d8ba1

Martin, thanks again for the patch, and my apologies for taking so long on the review turnaround.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8564032 [details] [diff] [review]
Implement helper functions to decode arbitrary aligned data in network byte order to host endianess

Review of attachment 8564032 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsNetUtil.h
@@ +2765,5 @@
>  bool NS_IsReasonableHTTPHeaderValue(const nsACString& aValue);
>  
>  /**
> + * Return a host endian value decoded from network byte order,
> + * accessed in an alignement safe way.

Please use the functions in mozilla/Endian.h instead:

mozilla::NetworkOrder::readUint16
mozilla::NetworkOrder::readUint32

Comment 10

[u408661]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #9)
> Please use the functions in mozilla/Endian.h instead:
> 
> mozilla::NetworkOrder::readUint16
> mozilla::NetworkOrder::readUint32

Wow, learned my new thing for today pretty early in the day! I've updated Martin's patch to use those, as well as switching over our custom write implementations to use the canonical versions. Running through try here https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6862e7952d

Patch upload imminent (in other words, once I can make git-bz work properly again, or give up and export manually).

Comment 11

[u408661]

Attachment: patch

Here's the updated patch, now using the canonical endianness converters.

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8594888 [details] [diff] [review]
patch

Review of attachment 8594888 [details] [diff] [review]:
-----------------------------------------------------------------

summary should probably read something like "manually align network integers for non-tier1 platforms". 

also, please drop the mozilla: namespace references on each call to NetworkEndian:foo(). You shouldn't need it at all, but if you do just add a single "using mozilla;"

Comment 13

[u408661]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4103833629c1

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4103833629c1