search

sumerc / yappi

Yet Another Python Profiler, but this time multithreading, asyncio and gevent aware.
MIT License
1.45k stars 73 forks source link

Context ID callback does not check if returned integer is a valid long #38

Closed sm-Fifteen closed 4 years ago

sm-Fifteen commented 4 years ago

If the callback passed to set_context_id_callback returns a number that is too large for a C long, yappi will throw an error along the lines of

Traceback (most recent call last):
  ...
  ...
  File "./test_bench.py", line 37, in dispatch
    tracked_stats[name] = yappi.get_func_stats({"name": call_to_track, "ctx_id": ctx_id})
  File "C:\Users\royrenn\Documents\prog\fastapi-test\.pyenv\lib\site-packages\yappi.py", line 1001, in get_func_stats
    stats = YFuncStats().get(filter=filter)
  File "C:\Users\royrenn\Documents\prog\fastapi-test\.pyenv\lib\site-packages\yappi.py", line 660, in get
    _yappi.enum_func_stats(self._enumerator, filter)
SystemError: <built-in function enum_func_stats> returned a result with an error set

with the following error sometimes(?) being logged:

[*]     [yappi-err]     context id callback returned non-integer

Having looked into it, this seems to be related to the use of PyLong_AsLong here:

https://github.com/sumerc/yappi/blob/7b3f08f5ad9d56e806ef2ddd9bf81a17f25f0fff/yappi/_yappi.c#L363-L376

I ended up running into this issue when running code similar to this on Windows. As it happens, id(my_object) is a 64 bits unsigned integer on 64-bits CPython, but a long on Windows is only 32 bits; only long longs are 64 bits. This could turn out to be an issue in other use cases, so something should probably be done to adress the potential mismatch between the range of Python ints and C types.

sumerc commented 4 years ago

Yes, you are right. I did not know Windows has a different view on long type.

One option to solve this problem might be to use PyLong_AsLongLong for APIs like set_ctx_id_cbk and set_tag_cbk and make sure we never shrink the value while passing internally(though it seems not much a problem since we use uintptr_t for passing these values all around)

Thanks for reporting this!

sumerc commented 4 years ago

I have a potential fix for the issue. Could you please also look at it? https://github.com/sumerc/yappi/pull/39

Thanks!

sm-Fifteen commented 4 years ago

Can confirm pip install --upgrade --force-reinstall https://github.com/sumerc/yappi/archive/fix-issue38.zip fixes the issue on my end.

sumerc commented 4 years ago

Nice! Thanks. Merged in master.