Skip to content

Fix undefined behavior in Buffer_AppendLongUnchecked#606

Merged
bwoodsend merged 3 commits into
ultrajson:mainfrom
WillAyd:ub-fix
Oct 1, 2023
Merged

Fix undefined behavior in Buffer_AppendLongUnchecked#606
bwoodsend merged 3 commits into
ultrajson:mainfrom
WillAyd:ub-fix

Conversation

@WillAyd

@WillAyd WillAyd commented Sep 13, 2023

Copy link
Copy Markdown
Contributor

This was flagged when running with UBSAN

@bwoodsend

Copy link
Copy Markdown
Collaborator

Looks like INT64_MIN isn't a thing on MSVC Windows. You might just have to #indef INT64_MIN; hardcode the values. Can you put some tests in so we can see what it is you're trying to fix and particularly, if this fix will behave on non-64bit machines.

@WillAyd

WillAyd commented Sep 22, 2023

Copy link
Copy Markdown
Contributor Author

Ah interesting - I'll have to look further into that. I fixed this downstream in pandas and didn't have the MSVC issue; must be something different with the compilation options

@WillAyd

WillAyd commented Sep 22, 2023

Copy link
Copy Markdown
Contributor Author

Ignore previous comment around test - for sure I'll add that

@WillAyd

WillAyd commented Sep 22, 2023

Copy link
Copy Markdown
Contributor Author

Bah sorry it is in the test suite already:

test_input = -9223372036854775808

@bwoodsend bwoodsend added the changelog: Fixed For any bug fixes label Sep 22, 2023
@codecov-commenter

codecov-commenter commented Sep 22, 2023

Copy link
Copy Markdown

Codecov Report

Merging #606 (71ea705) into main (6c2514e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 71ea705 differs from pull request most recent head f5e06e4. Consider uploading reports for the commit f5e06e4 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##             main     #606   +/-   ##
=======================================
  Coverage   91.66%   91.67%           
=======================================
  Files           6        6           
  Lines        1944     1946    +2     
=======================================
+ Hits         1782     1784    +2     
  Misses        162      162           
Files Changed Coverage Δ
lib/ultrajsonenc.c 85.60% <100.00%> (+0.07%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bwoodsend bwoodsend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rather disconcerting that we have a test specifically for this which didn't notice this issue...

@bwoodsend bwoodsend merged commit 0959d18 into ultrajson:main Oct 1, 2023
@bwoodsend

Copy link
Copy Markdown
Collaborator

Thanks!

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.

3 participants