As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 742833 - In WindowsDllInterceptor, write to the trampoline value as soon as it's valid
: In WindowsDllInterceptor, write to the trampoline value as soon as it's valid
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla14
Assigned To: Justin Lebar (not reading bugmail)
Depends on:
  Show dependency treegraph
Reported: 2012-04-05 11:56 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-04-11 09:16 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (2.00 KB, patch)
2012-04-05 15:28 PDT, Justin Lebar (not reading bugmail)
mh+mozilla: review+
Details | Diff | Splinter Review

Description User image Justin Lebar (not reading bugmail) 2012-04-05 11:56:28 PDT
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.  :)
Comment 1 User image Justin Lebar (not reading bugmail) 2012-04-05 11:59:14 PDT
Comment 2 User image Justin Lebar (not reading bugmail) 2012-04-05 15:28:26 PDT
Created attachment 612708 [details] [diff] [review]
Patch v1
Comment 3 User image Mozilla RelEng Bot 2012-04-05 16:47:34 PDT
Try run for 0310bf82b978 is complete.
Detailed breakdown of the results available here:
Results (out of 50 total builds):
    success: 44
    warnings: 6
Builds (or logs if builds failed) available at:
Comment 4 User image Mike Hommey [:glandium] 2012-04-10 00:01:41 PDT
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.
Comment 5 User image Matt Brubeck (:mbrubeck) 2012-04-11 09:16:30 PDT

Note You need to log in before you can comment on or make changes to this bug.