fastnumbers icon indicating copy to clipboard operation
fastnumbers copied to clipboard

[BUG] FastNumbers can crash with a SystemError due to returning NULL without setting an exception

Open lucventurini opened this issue 4 years ago • 4 comments

Describe the bug Observed in bug 274 of Mikado by myself and @fmarletaz: in certain conditions, FastNumbers crashes the program by returning NULL rather than None.

Expected behavior FastNumbers should return None or another valid Python value.

Environment (please complete the following information):

  • Python Version: 3.7
  • OS: LInux
  • Compiler (if known): @fmarletaz could check

To Reproduce Please see the bug above.

Suspicion is that in all instances where there is a return NULL

in fastnumbers.c (e.g. line 189), the correct line would be instead:

Py_RETURN_NONE

See here for why this is my suspicion.

lucventurini avatar Feb 28 '20 10:02 lucventurini

In a C-extension, there is no raise keyword, so the mechanism used to raise an exception is to set an exception, then return NULL from the extension function - this causes Python to raise the exception that was set. A SystemError is caused when one returns NULL but has not set an exception. I agree that it looks like there is a NULL returned somewhere without a set exception, but unfortunately the solution is not to blindly return None everywhere instead - for example, in the line you suggested returning None instead of NULL would cause None be returned instead of raising an exception if a user passed too many arguments to a function, passed an invalid keyword, passed and invalid type, etc.

I would like to help, but I do not have enough information to appropriately debug the problem at this time. Can you provide the following information:

  • The fastnumbers version that was experiencing the failure
  • An example input that triggers the failure

SethMMorton avatar Feb 28 '20 15:02 SethMMorton

@SethMMorton

Thank you for the quick answer. The version which triggers the error is the latest (3.0.0).

As for the input that causes the mistake, we think it was a rogue string in the input data. I cannot reproduce the error myself on my box, but I do not have access to the original data => there might be some weird ASCII/UTF character in the input which was not copied correctly over on GitHub.

For the time being, I have peppered the code with try/except when I am using fastnumbers, but it is a slightly suboptimal solution.

lucventurini avatar Feb 28 '20 16:02 lucventurini

The automated testing uses randomized input from across the whole UTF8, so my hope was that it should be robust against any input, but there is of course always the possibility that something slipped through.

I also remember going through the exercise once before of making sure no NULL could be returned without first setting an exception, but again it's possible I missed something.

Any help in narrowing this down will go a long way, otherwise it's a bit of a wild goose chase from my end.

SethMMorton avatar Feb 28 '20 17:02 SethMMorton

One way to help narrow this down is to ask the user of your code to modify it to print out the input sent to the fastnumbers function before the function is actually called, then run it on the offending input (or have them send you their input and you can run this yourself).

SethMMorton avatar Feb 28 '20 22:02 SethMMorton

I am going to close this because #45 should solve this problem - the return mechanism is cleaner so any problem where NULL can be returned erroneously should be caught now.

SethMMorton avatar Dec 14 '22 20:12 SethMMorton