Skip to content

API Change / Addition to sentry_capture_minidump#1138

Merged
supervacuus merged 21 commits intogetsentry:masterfrom
zsd4yr:master
Feb 10, 2025
Merged

API Change / Addition to sentry_capture_minidump#1138
supervacuus merged 21 commits intogetsentry:masterfrom
zsd4yr:master

Conversation

@zsd4yr
Copy link
Contributor

@zsd4yr zsd4yr commented Jan 31, 2025

See #1135

Copy link
Collaborator

@supervacuus supervacuus left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry using the header ##Unreleased if it is not there yet. Then, add a section **Breaking changes**: to list the changes.

@codecov
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.79%. Comparing base (03143a8) to head (f870493).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
+ Coverage   82.66%   82.79%   +0.12%     
==========================================
  Files          53       53              
  Lines        7927     7939      +12     
  Branches     1239     1239              
==========================================
+ Hits         6553     6573      +20     
+ Misses       1263     1256       -7     
+ Partials      111      110       -1     

…ntation.

The fix also visualizes the event_id generation and capture validity discrepancy much better.

The bug in the proposed implementation was retrieving the event_id from the envelope after it had been captured. In this case we have no control over the validity of the envelope because the capture function takes ownership and frees the envelope. Retrieving the event_id after the capture is thus a race that leads to a UAF.

We can only safely retrieve the event_id from the moment where we create the envelope from the event (which is also where the event_id is generated). Of course after this a few things can still go wrong, which is why we cannot maintain the event_id as return value, but only return it if we reached the capture.

In all other cases we return a nil UUID, as specified in the comment.
@zsd4yr
Copy link
Contributor Author

zsd4yr commented Feb 10, 2025

Looking into this failure in tests

FAILED tests/test_unit.py::test_unit[capture_minidump_basic] - subprocess.CalledProcessError: Command '['/Users/runner/Library/Android/sdk/platform-tools/adb', 'shell', 'cd /data/local/tmp && LD_LIBRARY_PATH=. ./sentry_test_unit --no-summary capture_minidump_basic; echo -n ret:$?']' returned non-zero exit status 1.

@zsd4yr
Copy link
Contributor Author

zsd4yr commented Feb 10, 2025

And these

FAILED tests/test_integration_http.py::test_capture_minidump - Failed: running command failed: valgrind --suppressions=/home/runner/work/sentry-native/sentry-native/tests/valgrind.txt --error-exitcode=33 --leak-check=yes ./sentry_example log attachment capture-minidump
FAILED tests/test_unit.py::test_unit[capture_minidump_basic] - Failed: running command failed: valgrind --suppressions=/home/runner/work/sentry-native/sentry-native/tests/valgrind.txt --error-exitcode=33 --leak-check=yes ./sentry_test_unit --no-summary capture_minidump_basic
FAILED tests/test_unit.py::test_unit[capture_minidump_without_sentry_init] - Failed: running command failed: valgrind --suppressions=/home/runner/work/sentry-native/sentry-native/tests/valgrind.txt --error-exitcode=33 --leak-check=yes ./sentry_test_unit --no-summary capture_minidump_without_sentry_init
FAILED tests/test_unit.py::test_unit_transport[capture_minidump_basic] - Failed: running command failed: valgrind --suppressions=/home/runner/work/sentry-native/sentry-native/tests/valgrind.txt --error-exitcode=33 --leak-check=yes ./sentry_test_unit --no-summary capture_minidump_basic
FAILED tests/test_unit.py::test_unit_transport[capture_minidump_without_sentry_init] - Failed: running command failed: valgrind --suppressions=/home/runner/work/sentry-native/sentry-native/tests/valgrind.txt --error-exitcode=33 --leak-check=yes ./sentry_test_unit --no-summary capture_minidump_without_sentry_init

@supervacuus
Copy link
Collaborator

Looking into this failure in tests

There is no need, @zsd4yr. I had to rewrite the proposed implementation (and parts of the initial implementation). I'll take over from here. Thanks!

…inside the options scope. Ensure to track even for faile item and failed envelope creation.
@zsd4yr
Copy link
Contributor Author

zsd4yr commented Feb 10, 2025

Thanks @supervacuus !

@supervacuus supervacuus merged commit 8040e6b into getsentry:master Feb 10, 2025
22 checks passed
@zsd4yr
Copy link
Contributor Author

zsd4yr commented Feb 12, 2025

@supervacuus Thanks for getting this across the finish line! When will the next drop be available? I'll update our fork with this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments