jborean93 / pyspnego

Python SPNEGO authentication library
MIT License
52 stars 11 forks source link

Access violation on Windows in sspi _dealloc_buffer() with heap allocation monitoring enabled #40

Closed rolyapples closed 2 years ago

rolyapples commented 2 years ago

pyspnego 0.5.2 with Python 3.9.7 on Windows 10:

  1. Using gflags, enable page heap allocation monitoring for python: "gflags /i python.exe +hpa"
  2. Run this bit of python:
    
    import spnego

context = spnego.client('myusername', 'mypassword', hostname='myhost', service='WSMAN', channel_bindings=None, context_req=spnego.ContextReq.default, protocol='negotiate', options=spnego.NegotiateOptions.wrapping_winrm)

out_token = context.step()

This gives me an access violation exception in the code generated from _dealloc_buffer in sspi - partial stack trace:

sspi_cp39_win_amd64!pyx_pf_6spnego_9_sspi_raw_4sspi_9SecBuffer_10_dealloc_buffer+0x14 [C:\temp\scratch\pyspnego-0.5.2\src\spnego_sspi_raw\sspi.c @ 5570] sspi_cp39_win_amd64!pyx_pw_6spnego_9_sspi_raw_4sspi_9SecBuffer_11_dealloc_buffer+0x1a [C:\temp\scratch\pyspnego-0.5.2\src\spnego_sspi_raw\sspi.c @ 5545] sspi_cp39_win_amd64!Pyx_PyObject_CallMethO+0x53 [C:\temp\scratch\pyspnego-0.5.2\src\spnego_sspi_raw\sspi.c @ 16517] sspi_cp39_win_amd64!Pyx_PyObject_CallNoArg+0x42 [C:\temp\scratch\pyspnego-0.5.2\src\spnego_sspi_raw\sspi.c @ 16583] sspi_cp39_win_amd64!pyx_pf_6spnego_9_sspi_raw_4sspi_9SecBuffer_12dealloc+0xcb [C:\temp\scratch\pyspnego-0.5.2\src\spnego_sspi_raw\sspi.c @ 5739] sspi_cp39_win_amd64!pyx_pw_6spnego_9_sspi_raw_4sspi_9SecBuffer_13dealloc+0x8 [C:\temp\scratch\pyspnego-0.5.2\src\spnego_sspi_raw\sspi.c @ 5702] sspi_cp39_win_amd64!pyx_tp_dealloc_6spnego_9_sspi_raw_4sspi_SecBuffer+0x64 [C:\temp\scratch\pyspnego-0.5.2\src\spnego_sspi_raw\sspi.c @ 11828] python39!_Py_Dealloc+0x16 [c:\python-3.9.7\objects\object.c @ 2209] python39!_Py_DECREF+0x1c [c:\python-3.9.7\include\object.h @ 430] python39!_Py_XDECREF+0x21 [c:\python-3.9.7\include\object.h @ 497] python39!list_dealloc+0xd9 [c:\python-3.9.7\objects\listobject.c @ 336] sspi_cp39_win_amd64!_Py_DECREF+0xc [C:\ProgramData\Miniconda3\envs\harness39_temp\include\object.h @ 430] sspi_cp39_win_amd64!pyx_tp_dealloc_6spnego_9_sspi_raw_4sspi_SecBufferDesc+0xa8 [C:\temp\scratch\pyspnego-0.5.2\src\spnego_sspi_raw\sspi.c @ 11638]

It's failing on this bit of the .pyx code:
`if self.c_value != NULL and self.c_value.pvBuffer != NULL and self.sys_alloc:`

Because __pyx_v_self->c_value->pvBuffer (from the generated C code) is inaccessible. It seems that Python might have freed some of this by the time it gets to run this code, or something like that.

This appears to be causing problems in an application that uses pypsrp (which uses spnego) - I see occasional crashes that appear to be due to memory corruption (this is without heap allocation monitoring enabled, in production environment). 

If I comment out these lines from sspi.pyx, the crashes no longer seem to happen:
    if self.c_value != NULL and self.c_value.pvBuffer != NULL and self.sys_alloc:
        FreeContextBuffer(self.c_value.pvBuffer)

    if self.c_value:
        self.c_value.pvBuffer = NULL

Note I'm not suggesting that as a fix for this - unfortunately I don't know Cython to suggest anything better.
jborean93 commented 2 years ago

Thanks for the detailed issue, I never knew about this mode. Will look further into it and come up with a fix.

jborean93 commented 2 years ago

I've opened a tentative PR that should fix this problem https://github.com/jborean93/pyspnego/pull/41. It moves a lot of the management of these buffers to the Python side of the code to manually call. It makes it a bit more involved there but it makes the Cython side more simpler and I don't need to worry about the class being partially destroyed when __deallocate__ is called.

rolyapples commented 2 years ago

@jborean93 many thanks for quick turnaround on this. As feedback, it looks like this has solved the occasional crashes I was seeing during normal operation, as well as behaving well when heap allocation monitoring is enabled - so all is good!

jborean93 commented 2 years ago

Thanks @rolyapples for confirming the fix. I've just published 0.5.3 on PyPI which includes these changes. Appreciate the bug report and the detailed information your provided. Was very helpful.