pkg/symbolizzer: use thread safe sync.Map to avoid concurrency issue

[jiangenj]

---

Before sending a pull request, please review Contribution Guidelines: https://github.com/google/syzkaller/blob/master/docs/contributing.md

---

[jiangenj]

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 GitHubhttps://github.com/google/syzkaller/pull/4787#pullrequestreview-2047341783, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BCHNAODNKJYFMZ2UOED3KWLZBMR2ZAVCNFSM6AAAAABHN5IBXGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBXGM2DCNZYGM. You are receiving this because you authored the thread.Message ID: @.@.>>

[dvyukov]

Please show the other changes where this is needed. Otherwise it's impossible to understand if this is the right approach or not. There should be some synchronization in the symbolizer, possibly the interner should be protected by the same synchronization. Symbolizer is also not used concurrently by manager now, so I would like to understand what you want to do with manager to use symbolizer concurrently. Frequently using synchronization in the lowest-level primitives is the wrong approach.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 6.38298% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 61.2%. Comparing base (f550015) to head (2786e60).

Additional details and impacted files

Files	Coverage Δ
pkg/symbolizer/cache.go	100.0% <100.0%> (ø)
pkg/cover/backend/dwarf.go	11.9% <0.0%> (-0.6%)	:arrow_down:

... and 1 file with indirect coverage changes

[jiangenj]

Please show the other changes where this is needed. Otherwise it's impossible to understand if this is the right approach or not. There should be some synchronization in the symbolizer, possibly the interner should be protected by the same synchronization. Symbolizer is also not used concurrently by manager now, so I would like to understand what you want to do with manager to use symbolizer concurrently. Frequently using synchronization in the lowest-level primitives is the wrong approach.

updated the PR, please check again.

[jiangenj]

@dvyukov Can you review this?

[jiangenj]

Any further comment for this PR since there is no pending item? @dvyukov

[dvyukov]

Any further comment for this PR since there is no pending item? @dvyukov

Sorry, did not realize this is ready for review. Please either comment something like "Please take another look", or press this button: