-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
all: parallelize symbolize #4787
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is the issue?
This type is not supposed to be used concurrently, perhaps there is a bug in the caller.
It's used in the symbolizer, which is not thread-safe either.
The current merged master branch doesn’t have issue.
But I have coming changes which will parallel to use the symbolizer, then it will report goroutine concurrency issue.
Those changes are a little big, so I submit this PR first as it doesn’t influence existing code anyway.
From: Dmitry Vyukov ***@***.***>
Sent: Thursday, May 9, 2024 3:39 PM
To: google/syzkaller ***@***.***>
Cc: Joey Jiao (QUIC) ***@***.***>; Author ***@***.***>
Subject: Re: [google/syzkaller] pkg/symbolizzer: use thread safe sync.Map to avoid concurrency issue (PR #4787)
@dvyukov requested changes on this pull request.
What exactly is the issue?
This type is not supposed to be used concurrently, perhaps there is a bug in the caller.
It's used in the symbolizer, which is not thread-safe either.
—
Reply to this email directly, view it on GitHub<#4787 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BCHNAODNKJYFMZ2UOED3KWLZBMR2ZAVCNFSM6AAAAABHN5IBXGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBXGM2DCNZYGM>.
You are receiving this because you authored the thread.Message ID: ***@***.******@***.***>>
|
Please show the other changes where this is needed. Otherwise it's impossible to understand if this is the right approach or not. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
updated the PR, please check again. |
@dvyukov Can you review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks good.
Thanks for fixing the race.
It's not safe to append to slice from multiple goroutines. Either using chan or lock can get constant result.
The PR is to fix these two issues:
Before sending a pull request, please review Contribution Guidelines:
https://github.com/google/syzkaller/blob/master/docs/contributing.md