Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exceptions on encoding list or dict elements and non-overflow errors on int handling getting silenced #505

Merged

Conversation

JustAnotherArchivist
Copy link
Collaborator

Fixes #273

I have no idea how to make PyLong_As* raise an exception other than OverflowError for testing that part, but it should be possible in theory (e.g. running out of memory at that exact moment, I guess?).

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2022

Codecov Report

Merging #505 (cd16c26) into main (c41db2a) will increase coverage by 0.09%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #505      +/-   ##
==========================================
+ Coverage   88.79%   88.88%   +0.09%     
==========================================
  Files           6        6              
  Lines        1695     1709      +14     
==========================================
+ Hits         1505     1519      +14     
  Misses        190      190              
Impacted Files Coverage Δ
tests/test_ujson.py 99.54% <ø> (ø)
python/objToJSON.c 84.59% <33.33%> (+0.07%) ⬆️
lib/ultrajsonenc.c 82.03% <100.00%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c41db2a...cd16c26. Read the comment docs.

@JustAnotherArchivist
Copy link
Collaborator Author

This is also relevant to #402. What might have been happening there was that the error was getting ignored and it kept appending commas until it ran out of buffer (cf. my comments on #504). While the segfault is no longer possible as far as I can tell (didn't track back where it was fixed though), this causes the first TypeError to get raised rather than the last one.

@JustAnotherArchivist JustAnotherArchivist marked this pull request as draft February 13, 2022 16:50
@JustAnotherArchivist
Copy link
Collaborator Author

JustAnotherArchivist commented Feb 14, 2022

I realised earlier that this was missing some cleanup on returning from encode. There was actually an extra tiny bug before this PR that hadn't been mentioned anywhere it seems:

>>> print(repr(ujson.dumps([2**64, 42], indent=1)))
'[\n ,\n  42\n ]'

There's an extra space on the third and fourth line because level isn't decremented in the JT_INVALID case.

This further led me to discover that beginTypeContext cleans up after itself iff returning JT_INVALID. But it still returns a type context, just with a null pointer for prv etc. This means it's not necessary to call endTypeContext from the JT_INVALID case, which is pretty unintuitive since there is a type context of some description. I refrained from trying to make endTypeContext idempotent though; apparently it's not as simple as testing whether tc->prv is a null pointer, and I don't have time right now to dig further into it.

@JustAnotherArchivist JustAnotherArchivist marked this pull request as ready for review February 14, 2022 03:26
@bwoodsend
Copy link
Collaborator

I have no idea how to make PyLong_As* raise an exception other than OverflowError for testing that part, but it should be possible in theory (e.g. running out of memory at that exact moment, I guess?).

An overflow error sounds sensible to me anyway.

@bwoodsend bwoodsend added the changelog: Fixed For any bug fixes label Feb 16, 2022
@bwoodsend bwoodsend merged commit 7f269a4 into ultrajson:main Feb 16, 2022
@JustAnotherArchivist
Copy link
Collaborator Author

@bwoodsend Just to be clear, I meant that those functions could in principle set another exception, and the previous code would've ignored them at least in certain scenarios since it only handled OverflowError. I wanted to add a test for that, i.e. that other exceptions get passed through, but I couldn't come up with anything. In any case, the new code should behave correctly in those weird circumstances.

Copy link
Contributor

@RouquinBlanc RouquinBlanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Wouldn't it be a bit nicer like this instead?

    if (exc && PyErr_ExceptionMatches(PyExc_OverflowError))
    {
      PyErr_Clear();
      pc->PyTypeToJSON = PyLongToUINT64;
      tc->type = JT_ULONG;
      GET_TC(tc)->unsignedLongValue = PyLong_AsUnsignedLongLong(obj);

      exc = PyErr_Occurred();
    }

    if (exc)
    {
      PRINTMARK();
      goto INVALID;
    }

@JustAnotherArchivist
Copy link
Collaborator Author

@RouquinBlanc Good point! Yes, that would be cleaner. Since this has already been merged, could you make a separate PR for that? Or would you prefer if I did it?

@RouquinBlanc
Copy link
Contributor

Done!

#510

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Fixed For any bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ujson does not raise overflow error when a second integer value is present
4 participants