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

RESOLVED FIXED in Firefox 40

Status

()

Core
Networking: HTTP
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Martin Husemann, Assigned: nwgh)

Tracking

35 Branch
mozilla40
Sun
NetBSD
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8560973 [details] [diff] [review]
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
(Reporter)

Comment 1

4 years ago
A helper function like:

     uint32_t PR_be32dec(const void *stream);

would clean this up quite a bit.
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.
Attachment #8560973 - Flags: review?(daniel)
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.
Flags: needinfo?(hurley)
(Assignee)

Comment 4

4 years ago
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 :)
Flags: needinfo?(hurley)
Attachment #8560973 - Flags: review?(daniel)
(Reporter)

Comment 5

4 years ago
Sure, might take a few days though.
(Reporter)

Comment 6

4 years ago
Created attachment 8564032 [details] [diff] [review]
Implement helper functions to decode arbitrary aligned data in network byte order to host endianess
Attachment #8560973 - Attachment is obsolete: true
Attachment #8564032 - Flags: review?(hurley)
(Reporter)

Updated

3 years ago
Attachment #8564032 - Flags: review?(hurley) → review?(michal.novotny)
(Assignee)

Comment 7

3 years ago
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)
Attachment #8564032 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 8

3 years ago
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 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
(Assignee)

Comment 10

3 years ago
(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).
(Assignee)

Comment 11

3 years ago
Created attachment 8594888 [details] [diff] [review]
patch

Here's the updated patch, now using the canonical endianness converters.
Attachment #8564032 - Attachment is obsolete: true
Attachment #8594888 - Flags: review?(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;"
Attachment #8594888 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/4103833629c1
Assignee: nobody → hurley
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.