[crashtracker] Enable dynamic updating of metadata and config at runtime #297
Conversation
|
The way that the "endpoint" gets configured in Ruby is also dynamic, and can also change when configuration via code is used. Would it be possible to also allow that to be reconfigured? |
crashtracker/src/crash_handler.rs
Outdated
| fn handle_posix_signal_impl(signum: i32) -> anyhow::Result<()> { | ||
| let mut receiver = match std::mem::replace(unsafe { &mut RECEIVER }, GlobalVarState::Taken) { | ||
| GlobalVarState::Some(r) => r, |
There was a problem hiding this comment.
One thing that occurred to me while reviewing the PR is -- should we have something protecting handle_posix_signal_impl so that it can't get called multiple times in the same process?
Afaik if a thread is handling a signal (e.g. SIGSEGV), no other SIGSEGVs can be delivered. But I... don't think that applies to other threads in the same process.
May be worth giving it a try?
danielsn
changed the title
| let old = METADATA.swap(box_ptr, SeqCst); | ||
| if !old.is_null() { | ||
| // Safety: This can only come from a box above. | ||
| unsafe { | ||
| std::mem::drop(Box::from_raw(old)); | ||
| } | ||
| } |
There was a problem hiding this comment.
It may be worth clearly documenting this design where we expect accesses to all these AtomicPtr to always be made through swap and never through load.
(Or introduce a quick wrapper class that takes care of this?)
Otherwise, when reading the code, you need to reverse engineer this important fact by checking every place where we access them.
crashtracker/src/crash_handler.rs
Outdated
| anyhow::ensure!( | ||
| matches!(old_receiver, GlobalVarState::Unassigned), | ||
| "Error registering crash handler receiver: receiver already existed {old_receiver:?}" | ||
| old_receiver.is_null(), | ||
| "Error registering crash handler receiver: receiver already existed" | ||
| ); |
There was a problem hiding this comment.
If the semantics here is to say an existing receiver already existing is an error, should we do a compare and swap on RECEIVER instead, expecting it to be null?
Right now it's kinda weird that we successfully set up the new receiver, and only then we return an error.
There was a problem hiding this comment.
I modified the semantics of registering a reciever
crashtracker/src/crash_handler.rs
Outdated
| pub fn register_crash_handlers(create_alt_stack: bool) -> anyhow::Result<()> { | ||
| anyhow::ensure!(OLD_HANDLERS.load(SeqCst).is_null()); | ||
| if !OLD_HANDLERS.load(SeqCst).is_null() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Minor: Not sure this is something we want to care about, but one thing we could do when the function is called is to check that our signal handlers are still active, e.g. because something else on the system may have overwritten them.
(I'm thinking of this, because there may be a situation where someone starts the crash tracker, and then discovers that the VM overwrites their signal handlers after X, and then calls register_crash_handlers again expecting libdatadog to be restored but isn't. Maybe too farfetched?)
What does this PR do?
Makes the
CrashtrackerMetadataandCrashtrackerConfigurationdynamically updatable, rather than fixed at crashtracker startup.Motivation
Ruby allows the user to update tags on the fly, so we need to support that.
Additional Notes
This will also be useful for a sidecar setup, where we would want to keep the metadata local until its time to upload.
How to test the change?
Run the
test_crashtest in crashtracker/api, and notice that the extra tag is output in the crash report.For Reviewers
@DataDog/security-design-and-guidance.