Skip to content

syscall handler mod: fix double-warn race condition#3338

Merged
sporksmith merged 1 commit intoshadow:mainfrom
sporksmith:already-warned
Jun 5, 2024
Merged

syscall handler mod: fix double-warn race condition#3338
sporksmith merged 1 commit intoshadow:mainfrom
sporksmith:already-warned

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

Detect when another thread has raced to warn about a missing syscall number before us, ensuring we really do only warn once about a particular missing syscall number.

This doesn't seem like a substantial problem in practice, but looking at this code again it seems silly not to do this slightly better handling.

Detect when another thread has raced to warn about a missing syscall
number before us, ensuring we really do only warn once about a
particular missing syscall number.

This doesn't seem like a substantial problem in practice, but looking at
this code again it seems silly not to do this slightly better handling.
@sporksmith sporksmith added this to the Code health and maintenance milestone Jun 5, 2024
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Jun 5, 2024
@sporksmith sporksmith requested a review from robgjansen June 5, 2024 16:39
@sporksmith sporksmith enabled auto-merge (squash) June 5, 2024 16:39
@sporksmith sporksmith merged commit 0412e0a into shadow:main Jun 5, 2024
// already-warned set. Also detect the (rare) case that another
// thread already warned after we released the read-lock above.
let has_already_warned = has_already_warned
|| WARNED_SET
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems backwards to me? insert should return true when it's added for the first time, which would mean has_already_warned should be false.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Doh, right. Will follow up with another fix.

Whenever I look at this code I'm tempted to just use a Mutex instead of a RwLock since it shouldn't be on the hot path, but always end up deciding "oh well, this one last tweak should fix it once and for all anyway"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah it originally used a mutex, but then was changed to RwLock during review. As far as I remember there wasn't a strong argument for either one.

I did refactor this into a separate module as part of #3332, but I'm not sure if/when I'll merge that.

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

Labels

Component: Main Composing the core Shadow executable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants