pywinrt / python-winsdk

Python package with bindings for Windows SDK
https://python-winsdk.readthedocs.io
MIT License
74 stars 8 forks source link

10 MB write to stream crashes Python #20

Closed wolfmanstout closed 1 year ago

wolfmanstout commented 1 year ago

Simple repro on Windows 11 Python 3.9 64-bit:

import winsdk.windows.storage.streams as streams
data_writer = streams.DataWriter()
data_writer.write_bytes(10000000 * [0])
print("Wrote data")

This works fine with winrt. Try doing 100 MB if that doesn't repro.

Event Viewer log:

Faulting application name: python.exe, version: 3.9.12150.1013, time stamp: 0x623bb3b2
Faulting module name: ucrtbase.dll, version: 10.0.22621.608, time stamp: 0xf5fc15a3
Exception code: 0xc0000005
Fault offset: 0x0000000000051370
Faulting process id: 0x0x3154
Faulting application start time: 0x0x1D8FCF6FC83A24B
Faulting application path: C:\Python39-64\python.exe
Faulting module path: C:\WINDOWS\System32\ucrtbase.dll
Report Id: ebaa31ca-9228-41f8-bb92-e7659f0879c2
Faulting package full name: 
Faulting package-relative application ID: 
dlech commented 1 year ago

FYI, this will be more efficient (and maybe avoid the crash?) if an object that supports the buffer protocol is passed instead of a list.

data_writer.write_bytes(10000000 * b'\x00')

But we should still fix the crash.

dlech commented 1 year ago

Bug was introduce by https://github.com/pywinrt/pywinrt/commit/9564a9964f29eb6af8639fa1a44654a2f5d8c6ea. Returning com_array as array_view apparently frees the underlying buffer since array_view doesn't have a move constructor.

wolfmanstout commented 1 year ago

Thank you for looking into this! FWIW, winrt actually doesn't allow passing in a bytes object as you suggest (although it does seem to resolve the issue with winsdk). winrt gives the following error:

>>> data_writer.write_bytes(10000000 * b'\x00')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: The parameter is incorrect.

I only mention this in case the goal is to provide an identical drop-in replacement for winrt.

wolfmanstout commented 1 year ago

Hi, when is your ETA on pushing out a new release with the fix? I had another bug filed from a user of my package concerned about winrt being unmaintained, and this is the only blocker for me to switch over. Thanks!

dlech commented 1 year ago

I've been putting off doing a release because there have been some breaking changes and I haven't decided yet if the tradeoffs of the performance improvements are worth the breaking changes yet or not. Hopefully I can find some time to come back to this again soonish.

dlech commented 1 year ago

Should be fixed in v1.0.0b8. Note that write_bytes() now takes a bytes-like object instead of a list.