<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 25, 2017 at 2:29 PM, Aaron Klotz <span dir="ltr"><<a href="mailto:aklotz@mozilla.com" target="_blank">aklotz@mozilla.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi everybody,<br>
<br>
Last week I discovered that we have (soon to be "had") several places in our code where we were declaring Windows APIs using an incorrect ABI with js-ctypes. [1]<br>
<br>
On 32-bit Windows builds, this is not a good thing.<br>
<br>
Most (but not all) Windows APIs are declared using the WINAPI macro (which maps to ctypes.winapi_abi). Using ctypes.default_abi on one of these functions could corrupt the stack of the calling thread. [2]<br>
<br>
In general, if you look up your desired Windows API function on MSDN and its declaration contains WINAPI, you should be using ctypes.winapi_abi. On the other hand, if WINAPI is not present, that does not necessarily mean that you may safely use ctypes.default_abi; check the declarations in the Windows SDK headers to be sure.<br>
<br>
Finally, if you are landing code that contains new js-ctypes declarations, please obtain an r+ from somebody who knows and understands the various ABIs. And please do not cargo-cult the ABI parameter from existing declarations. Let's try to prevent future occurrences. :-)<br></blockquote><div><br></div><div>This feels like the kind of thing that automated tests should detect. Can we devise an ESLint plugin or some such so machines can help us find bugs instead of relying on us unreliable humans? One would think we could maintain a dummy source file that #include's various Windows headers, feeds them to a preprocessor, and looks for __stdcall, etc in the output and correlates with ABI conventions from ctypes callers.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
<br>
Aaron<br>
<br>
<br>
[1] Search for whiteboard containing [ctypes.abi]if you want to see the bugs.<br>
<br>
[2] On Win32, most API functions pop their own parameters off the stack when they return. ctypes.default_abi assumes that the /caller/ must pop the parameters off the stack once the callee returns. If you specify ctypes.default_abi on one of the affected WINAPI functions, then both the caller and the callee pop, thus messing up the calling thread's stack pointer. This distinction goes away on Win64, but ctypes.winapi_abi always does the right thing, so please use it.<br>
<br>
Why haven't we been seeing crashes right and left because of this? I'm not sure. I don't have the time to investigate further (maybe somebody else knows?), but I suspect that either js-ctypes or libffi might be setting up extra stuff in the calling stack frame such that these double-pops have not been moving the stack pointer enough to cross any important boundaries. Whatever it is that made us so lucky, we shouldn't be relying on it! ;-)<br>
______________________________<wbr>_________________<br>
firefox-dev mailing list<br>
<a href="mailto:firefox-dev@mozilla.org" target="_blank">firefox-dev@mozilla.org</a><br>
<a href="https://mail.mozilla.org/listinfo/firefox-dev" rel="noreferrer" target="_blank">https://mail.mozilla.org/listi<wbr>nfo/firefox-dev</a><br>
</blockquote></div><br></div></div>