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.

Sign up for GitHub

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

Jump to bottom

tests/optional_mutex: check counter within a lock #507

Merged
merged 1 commit into from
Apr 6, 2024
Merged

tests/optional_mutex: check counter within a lock #507

merged 1 commit into from
Apr 6, 2024

Conversation

gperciva
Copy link
Member

No description provided.

@gperciva
Copy link
Member Author

I'm estimating that there's an 80% chance you decline this PR. Coverity-scan complains about this; we could change the code, or mark it as "false positive".

The variables value and counter are modified within mutex locks, but the final check is not protected. I figured that wasn't necessary, since the final check happens after pthread_join(), so there isn't any danger of synchronization problems. But if it's better code style to slap a mutex there as well, I'm happy to do so.

@cperciva
Copy link
Member

It's good "lock hygiene" to do this. I would be very surprised if any system implemented lock-free pthread_join, but theoretically even though a thread has exited we can't guarantee that values it has written to memory are visible to us until we acquire a lock.

cperciva
cperciva reviewed Jan 14, 2024
View reviewed changes
tests/optional_mutex/main.c Outdated
if ((value == 0) && (counter == counter_expected))
rc = 0;
else
rc = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Umm, what happens to this value on line 117?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I fixed this by going back to having an int match.

@gperciva gperciva force-pushed the coverity-scan-fix branch 2 times, most recently from db4f63a to f676d7f Compare January 15, 2024 01:13
@gperciva gperciva marked this pull request as ready for review January 15, 2024 01:14
@gperciva gperciva force-pushed the coverity-scan-fix branch 2 times, most recently from 40d0d7b to 48302fe Compare January 15, 2024 23:40
@gperciva gperciva marked this pull request as draft January 25, 2024 01:18
@gperciva gperciva marked this pull request as ready for review January 26, 2024 23:21
@gperciva gperciva force-pushed the coverity-scan-fix branch 2 times, most recently from 9daf04c to f2c863c Compare January 28, 2024 00:19
@gperciva
tests/optional_mutex: check counter within a lock
463daa0 
The variables "value" and "counter" are modified within mutex locks, so
it's good "lock hygiene" to do the same for the final check.

Reported by:	Coverity scan
@gperciva gperciva force-pushed the coverity-scan-fix branch from 8a14c0a to 463daa0 Compare March 1, 2024 21:46
@cperciva cperciva merged commit da85610 into master Apr 6, 2024
2 checks passed
@gperciva gperciva deleted the coverity-scan-fix branch April 7, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cperciva cperciva cperciva left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gperciva @cperciva