intrepidcs / python_ics

Library for interfacing with Intrepid devices in Python
MIT License
61 stars 29 forks source link

Potential Heap Corruption in ics.transmit_messages #111

Closed pierreluctg closed 2 years ago

pierreluctg commented 2 years ago

Heap corruption fatal exception can occur in ics.transmit_messages when using data over 64 bytes.

I agree that data length over 64 bytes is not valid, but I do not believe it should produce a fatal exception.

The code bellow will produce the errors as seen here

(Most frequent)

About to send message with data length: XX bytes. ... Windows fatal exception: code 0xc0000374

Current thread 0x00008318 (most recent call first):
  File "scratch_130.py", line 23 in tx
  File "scratch_130.py", line 40 in main
  File "scratch_130.py", line 46 in <module>

Process finished with exit code -1073740940 (0xC0000374)

or

About to send message with data length: XX bytes. ... 
Process finished with exit code -1073740791 (0xC0000409)

or

About to send message with data length: XX bytes. ... Windows fatal exception: access violation

Current thread 0x000035f4 (most recent call first):
  File "scratch_130.py", line 26 in tx
  File "scratch_130.py", line 43 in main
  File "scratch_130.py", line 49 in <module>

Process finished with exit code -1073741819 (0xC0000005)

Code to produce the issue

# Run with PYTHONMALLOC=debug and PYTHONFAULTHANDLER=1 to capture the traceback.
# This is not required to reproduce the issue.
import ics

def tx(device, data_len):
    message = ics.SpyMessage()
    message.Protocol = ics.SPY_PROTOCOL_CANFD
    message.ArbIDOrHeader = 0x123
    msg_data = (0,) * data_len
    # We are never setting this value to more than 64
    message.NumberBytesData = len(msg_data[:64])
    message.Data = tuple(msg_data[:8])
    if len(msg_data) > 8:
        message.ExtraDataPtrEnabled = 1
        message.ExtraDataPtr = tuple(msg_data)
    message.StatusBitField = 0
    message.StatusBitField2 = 0
    message.StatusBitField3 = 0
    message.NetworkID = 1

    print(f"About to send message with data length: {data_len} bytes. ... ", end='')
    try:
        ics.transmit_messages(device, message)
        print(f"Done!")
    except ics.RuntimeError:
        print("RuntimeError!")
        raise print(ics.get_last_api_error(device))

def main():
    device = ics.find_devices()[0]
    ics.open_device(device)

    try:
        # Try to send message with data length exceeding 64 bytes
        # It's not a valid data length but this should not cause fatal exception.
        for data_len in range(65, 300):
            # at some point we will get a fatal exception
            # Windows fatal exception: code 0xc0000374 (heap corruption)
            tx(device, data_len)
    finally:
        ics.close_device(device)

if __name__ == '__main__':
    main()

Events from Windows Event Viewer

Faulting application name: python.exe, version: 3.8.10150.1013, time stamp: 0x608fe38a
Faulting module name: ntdll.dll, version: 10.0.19041.1288, time stamp: 0xa280d1d6
Exception code: 0xc0000374
Fault offset: 0x00000000000ff199
Faulting process id: 0x3ff4
Faulting application start time: 0x01d7e87bda69c673
Faulting application path: C:\Python38-64\python.exe
Faulting module path: C:\WINDOWS\SYSTEM32\ntdll.dll
Report Id: ecdccc4a-edce-4964-9b1f-d98e2d607037
Faulting package full name: 
Faulting package-relative application ID: 
Faulting application name: python.exe, version: 3.8.10150.1013, time stamp: 0x608fe38a
Faulting module name: icsneo40.dll, version: 3.9.6.7, time stamp: 0x616f7c25
Exception code: 0xc0000409
Fault offset: 0x000000000014ab82
Faulting process id: 0x1940
Faulting application start time: 0x01d7e87bd52f5990
Faulting application path: C:\Python38-64\python.exe
Faulting module path: C:\WINDOWS\SYSTEM32\icsneo40.dll
Report Id: eb55a772-8d4e-427b-a79e-7162d242e16e
Faulting package full name: 
Faulting package-relative application ID: 
Faulting application name: python.exe, version: 3.8.10150.1013, time stamp: 0x608fe38a
Faulting module name: icsneo40.dll, version: 3.9.6.7, time stamp: 0x616f7c25
Exception code: 0xc0000005
Fault offset: 0x0000000000123b1c
Faulting process id: 0x892c
Faulting application start time: 0x01d7e87e8157cae0
Faulting application path: C:\Python38-64\python.exe
Faulting module path: C:\WINDOWS\SYSTEM32\icsneo40.dll
Report Id: f073fdd1-3f28-414a-96ec-5c334e5adcd6
Faulting package full name: 
Faulting package-relative application ID: 
drebbe-intrepid commented 2 years ago

@pierreluctg I'm looking into this now

drebbe-intrepid commented 2 years ago

@pierreluctg I've confirmed this is an issue inside icsneo40.dll itself. We allocate 64 max and then attempt to memcpy the length given in the struct. I'm writing up a bug internally for this.

drebbe-intrepid commented 2 years ago

@pierreluctg Would you prefer ics.transmit_messages() to truncate or fail (raise an exception) when over 64 bytes here?

pierreluctg commented 2 years ago

If NumberBytesData is correctly set to 64 (max), ExtraDataPtr should be able to point to a structure of at least 64 bytes. So in this case truncating is fine.

However, a more pythonic behavior would be to raise an exception.

I am ok with both.

drebbe-intrepid commented 2 years ago

This has been fixed upstream in icsneo40.dll so closing this out.