-
Notifications
You must be signed in to change notification settings - Fork 671
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
database: add applicable dbtests for linkeddb #3486
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Darioush Jalali <darioush.jalali@avalabs.org>
database/linkeddb/linkeddb.go
Outdated
@@ -281,7 +281,7 @@ func (ldb *linkedDB) getHeadKey() ([]byte, error) { | |||
func (ldb *linkedDB) putHeadKey(key []byte) error { | |||
ldb.headKeyIsUpdated = true | |||
ldb.updatedHeadKeyExists = true | |||
ldb.updatedHeadKey = key | |||
ldb.updatedHeadKey = slices.Clone(key) |
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.
On mobile right now - but I think there are other instances in the linkeddb that we need to perform a copy
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.
In Put
we hold a direct reference to key
which can be problematic:
oldHead.Previous = key
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.
I think putting the copy in putHeadKey
results in an unnecessary copy during Delete
as well. Maybe we could copy the key
in Put
once we know we are going to hold a reference to it (aka after we check that we aren't overwriting an existing node)?
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.
updated
Why this should be merged
Improves test coverage and fixes an edge case in linkeddb.
How this works
Splits the database tests into TestsBasic and Tests, where TestsBasic can run only with a KeyValueReaderWriterDeleter.
Note linkeddb doesn't really implement the interface for Iteratee, as the iteration is not guaranteed to be ordered.
Adds a key modification step to TestMemorySafetyDatabase which would have identified this edge case as the fuzzer does not seem to find it.
How this was tested
CI