Closed
Bug 1153090
Opened 9 years ago
Closed 9 years ago
Unaligned access in cert block list
Categories
(Core :: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: martin, Assigned: mgoodwin)
Details
Attachments
(3 files, 1 obsolete file)
928 bytes,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
974 bytes,
patch
|
keeler
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.36 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
Obvious bug on alignment critical architectures
Comment 1•9 years ago
|
||
Comment on attachment 8590636 [details] [diff] [review] Do not cast and dereference an arbitrary byte pointer, use memcpy instead Deferring to mgoodwin who added this in bug 1024809
Attachment #8590636 -
Flags: review?(mgoodwin)
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8590636 [details] [diff] [review] Do not cast and dereference an arbitrary byte pointer, use memcpy instead Review of attachment 8590636 [details] [diff] [review]: ----------------------------------------------------------------- Hi, many thanks for your help on this. The change itself looks good to me (it's always great to get a patch with a bug report) but there are a couple of things that I should point out: Firstly, it's important that a patch is formatted correctly. See this document for more information: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F more general guidance is available here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch Secondly, we usually make sure that patches are made against mozilla-central, then uplifted to other branches as necessary. Your patch, reformatted, works fine for versions earlier than 40 (so we can use that for uplift) but we'll need a separate patch for central too. I'll sort these things out for you this time; if you contribute again, It'd be great if you could follow the guidelines I've linked to above. Thanks again.
Attachment #8590636 -
Flags: review?(mgoodwin) → review-
Assignee | ||
Comment 3•9 years ago
|
||
Comment 4•9 years ago
|
||
Martin is still learning the ropes on how to properly upstream patches - i went through this in the past so i'm trying to help :) As mark said, using an hg clone of mozilla-central, working against trunk and producing hg patches (with MQ, so far..) with a properly formatted commit message helps a lot for getting a patch directly commited, instead of having the reviewer reformat it for you. It may feel tedious, but once you get used to it that works :)
Assignee | ||
Comment 5•9 years ago
|
||
Patch for uplift (prepared against aurora)
Assignee: nobody → mgoodwin
Attachment #8590636 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8592129 -
Flags: review?(dkeeler)
Assignee | ||
Updated•9 years ago
|
Attachment #8592144 -
Flags: review?(dkeeler)
Attachment #8592129 -
Flags: review?(dkeeler) → review+
Comment on attachment 8592144 [details] [diff] [review] Bug1153090_uplift.patch Review of attachment 8592144 [details] [diff] [review]: ----------------------------------------------------------------- Actually, it occurs to me that we should maybe be using sizeof(hash) instead of hard-coded '4'. r=me with that changed on this and the other patch.
Attachment #8592144 -
Flags: review?(dkeeler) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8592144 [details] [diff] [review] Bug1153090_uplift.patch Uplift to aurora and beta so that it makes ESR ? Approval Request Comment [Feature/regressing bug #]: 1024809 [User impact if declined]: unaligned access on alignment critical archs (NPOTB) [Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ac8f93de22cb [Risks and why]: none [String/UUID change made/needed]: none
Attachment #8592144 -
Flags: approval-mozilla-beta?
Attachment #8592144 -
Flags: approval-mozilla-aurora?
Comment 9•9 years ago
|
||
Oops, sorry martin i sort of stole your authorship when importing your patch :(
(In reply to Landry Breuil (:gaston) from comment #7) > https://hg.mozilla.org/integration/mozilla-inbound/rev/bb16a021fcef I guess I wasn't clear - I meant replace all hard-coded '4's in that function with sizeof(hash) (or have a temporary variable). Updating the comment to say 'the size of the hash key' instead of 32 bits might also be good.
Comment 11•9 years ago
|
||
Oops, my bad.. so, something like this, reformatted to fit in 80 chars ?
Attachment #8592409 -
Flags: review?(dkeeler)
Attachment #8592409 -
Attachment is patch: true
Comment on attachment 8592409 [details] [diff] [review] bug-1153090-fwup Review of attachment 8592409 [details] [diff] [review]: ----------------------------------------------------------------- Sure - another option would be to use a temporary variable to cut down on the number of parentheses, but I think this is fine.
Attachment #8592409 -
Flags: review?(dkeeler) → review+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bb16a021fcef https://hg.mozilla.org/mozilla-central/rev/ff2912e36af0
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Comment 15•9 years ago
|
||
Comment on attachment 8592144 [details] [diff] [review] Bug1153090_uplift.patch Should be in 38 beta 5. > [Risks and why]: none All patches have risks. Please provide some evaluation next time!
Attachment #8592144 -
Flags: approval-mozilla-beta?
Attachment #8592144 -
Flags: approval-mozilla-beta+
Attachment #8592144 -
Flags: approval-mozilla-aurora?
Attachment #8592144 -
Flags: approval-mozilla-aurora+
Comment 17•9 years ago
|
||
This time with 100% less silly uplift bustage. https://hg.mozilla.org/releases/mozilla-aurora/rev/27d913eaef22
You need to log in
before you can comment on or make changes to this bug.
Description
•