Opened 8 years ago

Closed 8 years ago

#5430 enhancement closed fixed (fixed)

Declare the signature of WSAAddressToString in _win32ifaces.py

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: core Keywords: win64
Cc: Branch: branches/wsaatf-5430
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

The WSAAddressToStringA function used by win32GetLinkLocalIPv6Address in twisted/internet/test/_win32ifaces.py, added for #5084, does not have its signature declared. ctypes very helpfully makes an (incorrect) guess, leading to an intermittent WSAEFAULT failure.

The 3rd argument, explicitly passed a null pointer (in the form of an integer 0), sometimes ends up as a non-null pointer in the underlying call. ctypes presumably only manages to copy four bytes worth of zeros into a variable somewhere, and sometimes the upper four bytes of that variable - since Win64 pointers are 8 bytes - contains non-zero garbage.

When this happens, the call typically fails, because the pointer is probably not to a mapped memory region (and even if it is, it would still lead to some wrong behavior).

Declaring the signature of WSAAddressToString will let ctypes know this is a pointer argument, which will let it fully initialize a null pointer, which should fix the WSAEFAULT. This also removes the workaround added, allocating 16kB for buf2. That value can be reduced to something like 64 bytes.

Attachments (1)

wsaaddresstostring-argtypes.patch (933 bytes) - added by Jean-Paul Calderone 8 years ago.

Download all attachments as: .zip

Change History (8)

Changed 8 years ago by Jean-Paul Calderone

comment:1 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added

PenguinOfDoom actually tracked down this flaw and authored the attached patch.

There are no unit tests for this fix, as this kind of bogus memory initialization isn't something we have the tools to reliably unit test.

comment:2 Changed 8 years ago by Glyph

Author: glyph
Branch: branches/wsaatf-5430

(In [33359]) Branching to 'wsaatf-5430'

comment:3 Changed 8 years ago by Glyph

I took the liberty of producing some build results. Comments forthcoming.

comment:4 Changed 8 years ago by Glyph

Owner: set to Glyph
Status: newassigned

Reviewin'.

comment:5 Changed 8 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to Jean-Paul Calderone
Status: assignednew

Thanks a lot, exarkun and PenguinOfDoom, for figuring this out. It is pretty terrible. I'm glad that this was ultimately our fault and not some kernel craziness.

Comments about the change itself:

  1. There really needs to be a link to the canonical reference for how that random little list of ctypes was derived. I think this is the right place?
  2. Even with a link to the reference, the types might be a little mysterious. I'd really like to have something I can eyeball and compare with the reference as mechanically as possible, then something that associates this with something else. For example, something like:
    LPSOCKADDR = c_void_p           # lpsaAddress
    DWORD = c_int                   # dwAddressLength
    # ...
    WSAAddressToString.argtypes = [LPSOCKADDR, DWORD] # etc.
    
    Better yet get the actual arg names in there.
  3. I know the default restype is c_int, but why not set it anyway, just so it's explicit and nobody thinks we forgot it?
  4. WSAIoctl doesn't ahve its argtypes declared either. Looking at the reference docs for the signature, I'm pretty sure I still see a couple of pointers being passed as 32-bit ints. (Please, set its restype as well.)

As far as testing goes, I'm drawing a total blank. I think that eyeballing this repeatedly is as good as we're likely to get. I wish I had a better answer.

So I'd say the fix is ready to land if you see fit, as long as you address point 1. If you feel like tackling point 2 too, go for it.

Now for some comments about the surrounding code, which you may want to address some other how:

  1. WinDLL is like an import. Why is it happening at function scope rather than global scope? If it actually does need to stay there, there should really be a comment as to why. Presumably this module should never be imported on a non-Windows platform anyway. (If these really do need to be module level, I'd actually really prefer it if you could just define another top-level function for each function so that we can hide away some of the ctypes crud; this function is long enough as it is and as we add more argtypes and restypes this is only going to get longer.)
  2. WSAAddressToString failure and WSAIoctl failure are pretty non-descriptive messages. Can you at least get the error value in there? Possibly the arguments that caused the error value? I am anticipating that we'll really want to see those if we're ever fielding support requests for this code or anything related to it.
  3. While we're on those errors, wouldn't WindowsError be more appropriate?
  4. This API should eventually be public. Interface enumeration is super helpful in some situations, and as this code demonstrates, super tricky. Is there a ticket for that?
  5. In the comment about WSAIoctl: "which means there was not enough space in the buffer we have it."

Thanks again. You can leave all of this latter feedback for later (except maybe the addition of WSAIoctl.restypes if it's not actually safe already).

comment:6 Changed 8 years ago by Jean-Paul Calderone

Addressed points 1, 2, 3, 4, 5, and 9. I'll file a ticket for 6, 7, and 8.

comment:7 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [33367]) Merge wsaatf-5430

Author: PenguinOfDoom, exarkun Reviewer: glyph Fixes: #5430

Clean up the ctypes usage in the IPv6 link local interface lookup code for Windows, fixing a bug on 64 bit Windows where a bad pointer will sometimes be passed to WSAAddressToString. Make some other changes to try to make the code more maintainable in general, too.

Note: See TracTickets for help on using tickets.