Run Miri on profiling crates and fix miri issues + Introduce ddog_prof_Profile struct by danielsn · Pull Request #235 · DataDog/libdatadog

danielsn · 2023-08-29T20:31:12Z

What does this PR do?

This PR is a followup to #225.

It adds runs Miri on all feasible tests in profiling and profiling-ffi.
It fixes UB in Miri found in some profiling-ffi tests
It updates the CI action to run these tests on all future PRs.

While changing this code, we (DSN and Levi) felt it was better to make a few changes to make this easier to reason about safety:

Introduce a ddog_prof_Profile struct that wraps a pointer to the Rust datadog_profiling::profile::Profile object, and return this from ddog_prof_Profile_new instead of ddog_prof_Profile*. This means you need to take the address for further operations e.g:

ddog_prof_Profile profile = ddog_prof_Profile_new(/*...*/);
ddog_prof_Profile_add(&profile, /*...*/);
ddog_prof_Profile_drop(&profile);

Add ddog_prof_Profile_Result for errors which don't return anything on success, but return a ddog_Error when they fail. These functions have had their return type changed to use this result, and ddog_prof_Profile_UpscalingRuleAddResult and ddog_prof_Profile_AddResult were removed:
- ddog_prof_Profile_add (previously ddog_prof_Profile_AddResult)
- ddog_prof_Profile_set_endpoint (previously void)
- ddog_prof_Profile_add_endpoint_count (previously void)
- ddog_prof_Profile_add_upscaling_rule_poisson (previously ddog_prof_Profile_UpscalingRuleAddResult)
- ddog_prof_Profile_add_upscaling_rule_proportional (previously ddog_prof_Profile_UpscalingRuleAddResult)
- ddog_prof_Profile_reset (previously bool)

Motivation

Miri is a great way to find UB. We shouldn't have UB we can prevent.

Additional Notes

There was a subtle stacked-borrows violation cause by the ffi destructor taking an Option<&mut> instead of a *mut.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b3fb4e106c1d19183c831f38389f2658
shows how this plays out on a small example in the playground (use the tools/miri option to compare fails vs doesnt_fail.

How to test the change?

Update your code to account for the C API changes, but semantically nothing changed, so existing tests should work otherwise.

For Reviewers

If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

danielsn · 2023-08-30T00:54:10Z

+        // Leaving a null will help with double-free issues that can
+        // arise in C. Of course, it's best to never get there in the
+        // first place!
+        let raw = std::mem::replace(&mut self.inner, std::ptr::null_mut());


since null is default, I think you can just std::mem::take here

I tried that first, but trait Default is apparently not implmented for *mut _!

danielsn · 2023-08-30T00:55:36Z

-/// The `profile` can be null, but if non-null it must point to a valid object
-/// created by the Rust Global allocator.
+/// The `profile` can be null, but if non-null it must point to a Profile
+/// made by this module, which has not previously been dropped.


I like this comment better :)

ivoanjo

👍 LGTM, thanks for making our ffi safer and more strictly checked :)

ivoanjo · 2023-08-30T09:58:27Z

+    # We need to disable isolation because 
+    # "unsupported operation: `clock_gettime` with `REALTIME` clocks not available when isolation is enabled" 


❤️ for documenting this here :)

ivoanjo · 2023-08-30T10:00:33Z

  const ddog_prof_Period period = {wall_time, 60};
-  std::unique_ptr<ddog_prof_Profile, Deleter> profile{
+  ddog_prof_Profile profile_data{
      ddog_prof_Profile_new(sample_types, &period, nullptr)};
+  std::unique_ptr<ddog_prof_Profile, Deleter> profile{&profile_data};


Minor: I wonder if such a simple example needs this kind of sophistication, e.g. maybe we could simplify this a bit?

I just fixed it up for the changes I made ^_^

I get that in C++, they want to have destructors run, rather than doing it manually. But obviously the C bindings don't have destructors.

I think we can maybe simplify this code, or acknowledge that we care more about C++ and try to also generate C++ headers and bindings, or something.

ivoanjo · 2023-08-30T10:13:46Z

    /// Resets all data except the sample types and period. Returns the
    /// previous Profile on success.
-    pub fn reset(&mut self, start_time: Option<SystemTime>) -> Option<Profile> {
+    pub fn reset(&mut self, start_time: Option<SystemTime>) -> anyhow::Result<Profile> {


Minor: Is it still worth returning the old profile in this situation? As an API think, it's kinda weird -- when would we want to keep around the old profile after a reset?

I need to get this release moving so let's talk about that for v5.0. I think originally I was hoping to do something like:

let old = profile.reset(); // owned, not borrowed let pprof_profile = pprof::Profile::from(old);

But that didn't work for whatever reason and forgot to revisit. Or may it's that since the structures don't match anymore (did at one time), there's not really value to take ownership of the profile::Profile, since it has to copy things anyway.

danielsn added 2 commits August 29, 2023 16:22

Comment out the two tests miri should not run on in profiling

71bc98e

profiling-ffi works with miri

a5423d9

danielsn requested a review from a team as a code owner August 29, 2023 20:31

github-actions Bot added profiling Relates to the profiling* modules. ci-build labels Aug 29, 2023

danielsn force-pushed the dsn/miri-everywhere branch 6 times, most recently from c59432d to d873337 Compare August 29, 2023 20:39

Miri CI changes

c152e1e

danielsn force-pushed the dsn/miri-everywhere branch from d873337 to c152e1e Compare August 29, 2023 20:40

morrisonlevi reviewed Aug 29, 2023

View reviewed changes

Comment thread profiling-ffi/src/profiles.rs Outdated

also do profiling-ffi

5149101

danielsn force-pushed the dsn/miri-everywhere branch from 9b0ac37 to 5149101 Compare August 29, 2023 20:53

danielsn added 4 commits August 29, 2023 16:58

Only check given paths

64df559

UNDO THIS: confirm that we catch a bug

248a1d8

Undo the change, now that the bug was indeed caught

09cd64f

Fix the null-pointer issue

a5727f2

morrisonlevi force-pushed the dsn/miri-everywhere branch from 581fb13 to 9d396eb Compare August 29, 2023 23:23

refactor: profiling-ffi Profile and ProfileResult

90170f4

morrisonlevi force-pushed the dsn/miri-everywhere branch from 9d396eb to 90170f4 Compare August 29, 2023 23:25

morrisonlevi changed the title ~~Run Miri on all of profiling and fix the issues it found~~ Run Miri on profiling crates and fix miri issues, bump profiling version numbers Aug 29, 2023

morrisonlevi changed the title ~~Run Miri on profiling crates and fix miri issues, bump profiling version numbers~~ Run Miri on profiling crates and fix miri issues, bump version numbers Aug 30, 2023

morrisonlevi changed the title ~~Run Miri on profiling crates and fix miri issues, bump version numbers~~ Run Miri on profiling crates and fix miri issues Aug 30, 2023

morrisonlevi mentioned this pull request Aug 30, 2023

Bump ddcommon, ddtelemetry, and profiling crates to v4.0.0 #236

Merged

2 tasks

danielsn commented Aug 30, 2023

View reviewed changes

ivoanjo approved these changes Aug 30, 2023

View reviewed changes

morrisonlevi merged commit e1789d5 into main Aug 30, 2023

morrisonlevi deleted the dsn/miri-everywhere branch August 30, 2023 14:59

ivoanjo added a commit to DataDog/dd-trace-rb that referenced this pull request Aug 31, 2023

WIP update StackRecorder with changes from DataDog/libdatadog#235

5fe6994

ivoanjo changed the title ~~Run Miri on profiling crates and fix miri issues~~ Run Miri on profiling crates and fix miri issues + Introduce ddog_prof_Profile struct Sep 19, 2023

ivoanjo mentioned this pull request Sep 19, 2023

refactor!(profiling-ffi): ddog_prof_Profile_new can fail #252

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Run Miri on profiling crates and fix miri issues + Introduce ddog_prof_Profile struct#235

Run Miri on profiling crates and fix miri issues + Introduce ddog_prof_Profile struct#235
morrisonlevi merged 9 commits into
mainfrom
dsn/miri-everywhere

danielsn commented Aug 29, 2023 •

edited by morrisonlevi

Loading

Uh oh!

Uh oh!

danielsn Aug 30, 2023

Uh oh!

morrisonlevi Aug 30, 2023 •

edited

Loading

Uh oh!

danielsn Aug 30, 2023

Uh oh!

ivoanjo left a comment

Uh oh!

ivoanjo Aug 30, 2023

Uh oh!

ivoanjo Aug 30, 2023

Uh oh!

morrisonlevi Aug 30, 2023

Uh oh!

ivoanjo Aug 30, 2023

Uh oh!

morrisonlevi Aug 30, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		# We need to disable isolation because
		# "unsupported operation: `clock_gettime` with `REALTIME` clocks not available when isolation is enabled"

Uh oh!

Conversation

danielsn commented Aug 29, 2023 • edited by morrisonlevi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

How to test the change?

For Reviewers

Uh oh!

Uh oh!

danielsn Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

morrisonlevi Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielsn Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

ivoanjo Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

ivoanjo Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

morrisonlevi Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

ivoanjo Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

morrisonlevi Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danielsn commented Aug 29, 2023 •

edited by morrisonlevi

Loading

morrisonlevi Aug 30, 2023 •

edited

Loading

morrisonlevi Aug 30, 2023 •

edited

Loading