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

fix: unprotected read data race #998

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: unprotected read data race #998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
646fbcc
fix data race
corverroos Oct 20, 2024
52d81e7
use safeGetLatestVersion
corverroos Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion immutable_tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func (t *ImmutableTree) Get(key []byte) ([]byte, error) {
// If the tree is of the latest version and fast node is not in the tree
// then the regular node is not in the tree either because fast node
// represents live state.
if t.version == t.ndb.latestVersion {
if t.version == t.ndb.safeGetLatestVersion() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can use t.ndb.getLatestVersion() (thread safe)

return nil, nil
}

Expand Down
9 changes: 9 additions & 0 deletions nodedb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,6 +797,15 @@ func (ndb *nodeDB) resetLegacyLatestVersion(version int64) {
ndb.legacyLatestVersion = version
}

// safeGetLatestVersion returns the value of ndb.latestVersion directly
// without additional logic performed by getLatestVersion.
func (ndb *nodeDB) safeGetLatestVersion() int64 {
ndb.mtx.Lock()
defer ndb.mtx.Unlock()

return ndb.latestVersion
}

Comment on lines +800 to +808
Copy link

@coderabbitai coderabbitai bot Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

Direct accesses to latestVersion need to be protected

Several unprotected accesses to latestVersion were found that could lead to data races:

  • In nodedb_test.go: Multiple direct writes and reads to ndb.latestVersion without mutex protection
  • In import.go: Direct read of tree.ndb.latestVersion without mutex protection
  • In mutable_tree.go: Direct read in condition version == tree.ndb.latestVersion

These locations should be updated to use the new safeGetLatestVersion() method or ensure proper mutex synchronization.

🔗 Analysis chain

LGTM! Thread-safe implementation for accessing latestVersion.

The new method correctly implements thread-safe access to latestVersion using mutex synchronization.

Let's verify that all direct accesses to latestVersion have been replaced with this new safe method:

Let me analyze the direct accesses to verify if they are properly protected. I'll check the mutex usage patterns in these locations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining direct access to latestVersion that might not be protected
# We should only see accesses within methods that already hold the mutex

rg -A 3 "ndb\.latestVersion\s*[^=]*" --type go

Length of output: 4880

Script:

#!/bin/bash
# Check mutex lock/unlock patterns around these accesses
# Looking for mtx.Lock() calls before these accesses

# First, let's check nodedb.go for mutex patterns
rg -B 5 "mtx\.(Lock|Unlock)" nodedb.go

# Then, check if the test file has any mutex protection
rg -B 5 "mtx\.(Lock|Unlock)" nodedb_test.go

# Finally, check mutable_tree.go for the context of that access
rg -B 10 "version == tree\.ndb\.latestVersion" mutable_tree.go

Length of output: 6644

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm, interesting, @cool-develope should I do as suggested here?

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, will check quickly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Importer is OK since it is a fresh tree, I think mutable_tree.GetVersioned should be updated with getLatestVersion.

func (ndb *nodeDB) getLatestVersion() (int64, error) {
ndb.mtx.Lock()
latestVersion := ndb.latestVersion
Expand Down