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

feat: extend API with ptr/len-string interfaces #827

Merged
merged 36 commits into from
Apr 25, 2023

Conversation

supervacuus
Copy link
Collaborator

Fixes #819.

@github-actions
Copy link

github-actions bot commented Mar 29, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 5a4664a

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #827 (2b3a5f6) into master (1788653) will increase coverage by 1.37%.
The diff coverage is 76.71%.

❗ Current head 2b3a5f6 differs from pull request most recent head 16c8c30. Consider uploading reports for the commit 16c8c30 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
+ Coverage   81.20%   82.57%   +1.37%     
==========================================
  Files          53       53              
  Lines        6981     7301     +320     
  Branches     1119     1179      +60     
==========================================
+ Hits         5669     6029     +360     
+ Misses       1201     1165      -36     
+ Partials      111      107       -4     

@supervacuus
Copy link
Collaborator Author

Hi @Swatinem, can you give this a first once-over when you have time? (it is not urgent because I am unsure if I will have time to continue working on the PR before the easter holidays).

What the PR should do is rather apparent, but I was experimenting with how to de-duplicate the two paths in the code base. Sometimes the most trivial approach is to stay with two separate implementations (if the line count is low). Sometimes, it makes sense to express one in terms of the other. Sometimes I felt it would make the most sense to create a macro where parameters and parameter-dependent function calls are parameterized.

When implementing one in terms of the other, I typically wrote the c-string interface in terms of the byte-string one because you only pay for an additional NULL-check and strlen(), which you'd have to do anyway later on if you don't end up in an error branch, vs. potentially paying double for allocation.

Could you tell me whether the macros make sense and whether you have different implementation favorites for those instances? The PR is incomplete; I want to add even more unit tests (some of the previous functions were "covered" but not tested, revealing a few places that led to NULL-induced access violations). I also haven't yet touched the windows-specific interfaces.

Thx!

@supervacuus
Copy link
Collaborator Author

Also fixes: #831

@kahest kahest requested a review from markushi April 19, 2023 08:21
@supervacuus
Copy link
Collaborator Author

The remaining macOS CI build also fails on master: https://github.com/getsentry/sentry-native/actions/runs/4542271760/jobs/8420339196; last week, the macOS 11 image was updated. Of course, that doesn't mean there is no leak in the code, just that a change in this PR didn't introduce it.

I could not repro the leak using leaks on macOS, and the LLVM ASAN for aarch64 has some issues. But I will investigate further. Otherwise, I am open to any review suggestions :-)

@supervacuus
Copy link
Collaborator Author

@supervacuus
Copy link
Collaborator Author

supervacuus commented Apr 19, 2023

haha, what? https://github.com/getsentry/sentry-native/actions/runs/4742464992/jobs/8420773714?pr=827 🤦

The previous CI run from 1h ago still used the image version 20230411.1 (from last week), whereas this one used 20230318.1 (from yesterday? last month). Not all runners get the latest same, though, as this re-run on master shows: https://github.com/getsentry/sentry-native/actions/runs/4542271760/jobs/8421381133.

🤷 something, something eventual consistency. I err on the "this is not a real leak side".

Makefile Outdated Show resolved Hide resolved
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Nice one, please have a look at my comments!

src/sentry_string.h Outdated Show resolved Hide resolved
src/sentry_utils.c Show resolved Hide resolved
src/path/sentry_path_windows.c Outdated Show resolved Hide resolved
src/path/sentry_path_windows.c Outdated Show resolved Hide resolved
src/transports/sentry_transport_curl.c Outdated Show resolved Hide resolved
@supervacuus supervacuus merged commit e97930a into master Apr 25, 2023
@supervacuus supervacuus deleted the feat/add_ptr_len_str_api branch April 25, 2023 13:55
@supervacuus
Copy link
Collaborator Author

LSAN correctly found a leak, but it is outside our control:

  • our build on CI picks up the libcurl library of the MacOSX 12.1 SDK from XCode 13.2.1, which has version 7.77.0,
  • the following fix was provided in 7.78.0
  • which rectifies a leak introduced by a feature that was introduced with 7.77.0 (i.e., our version)

It is unclear why the build suddenly broke with the new runner images because neither XCode/SDK (and its libcurl version) nor llvm/clang/lsan where updated from previous builds.

supervacuus added a commit that referenced this pull request Jun 25, 2024
Also, try to remove the suppression for libcurl 7.77.0 added here:

#827 (comment)

since the libcurl version retrieved from the SDK is now 7.85.0
supervacuus added a commit that referenced this pull request Jun 26, 2024
* chore: Update crashpad to 2024-06-11

* update macos test images to 14 (adding one for 12)

* fix breakpad-backend compiler break in macOS >= 12 GHA runner image.

* fix crashpad xcode-llvm breakage

* fix quote typo

* Use macOS 13 instead of 12 as the older fallback

* move the macOS asan build to macOS 12

* Try to inject `-fexperimental-library` for the llvm clang asan build

* Remove build-step macos llvm asan build

* Add experimental library via cmake.py

* add the library search path for the clang experimental lib

* Get rid of the warning noise

* Ignore realizeClassWithoutSwift leaks

* document asan suppression

Also, try to remove the suppression for libcurl 7.77.0 added here:

#827 (comment)

since the libcurl version retrieved from the SDK is now 7.85.0

* re-introduce SCDynamicStoreCopyProxies suppression

* update crashpad to getsentry head
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.

Feature request: Add support for string length parameters
2 participants