Closed
Bug 742833
Opened 13 years ago
Closed 13 years ago
In WindowsDllInterceptor, write to the trampoline value as soon as it's valid
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
Details
Attachments
(1 file)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
In bug 741540 comment 0, it was noted that the WindowsDllInterceptor assigns to its return value later than it needs to.
Assigning to the trampoline earlier doesn't make us totally thread-safe, but it does mitigate a large race.
If none of this makes sense, the patch may help. :)
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #612708 -
Flags: review?(mh+mozilla)
Comment 3•13 years ago
|
||
Try run for 0310bf82b978 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=0310bf82b978
Results (out of 50 total builds):
success: 44
warnings: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-0310bf82b978
Comment 4•13 years ago
|
||
Comment on attachment 612708 [details] [diff] [review]
Patch v1
Review of attachment 612708 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/xre/nsWindowsDllInterceptor.h
@@ +350,5 @@
>
> memcpy(tramp, origFunction, nBytes);
>
> + // The trampoline is now valid.
> + *outTramp = tramp;
The trampoline is actually not valid at this point. It is only valid after all the tramp[...] assignments that create a jump into the original function. However, it doesn't really matter, because the hook function won't be called before the original function is patched to call the hook function, which is done later than that.
I'd either change the comment or move this assignment for good measure.
Attachment #612708 -
Flags: review?(mh+mozilla) → review+
Comment 5•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in
before you can comment on or make changes to this bug.
Description
•