Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

fix: should use copied value of iterator #33

Merged
merged 2 commits into from
Dec 2, 2021
Merged

Conversation

egonspace
Copy link
Contributor

We found the cause of this system crash(Finschia/finschia-sdk#314).

In this PR(#22), it was modified not to copy when getting a value from an iterator, but this code has a problem.

In rocksdb, iterator.value is a memory within db, so we don't know when it will be free. Therefore, it must be copied and used.

@egonspace egonspace self-assigned this Nov 26, 2021
@egonspace egonspace added the bug Something isn't working label Nov 26, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2021

Codecov Report

Merging #33 (1e7416d) into main (fcfa67d) will increase coverage by 13.88%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #33       +/-   ##
===========================================
+ Coverage   52.60%   66.49%   +13.88%     
===========================================
  Files          29       28        -1     
  Lines        1785     1158      -627     
===========================================
- Hits          939      770      -169     
+ Misses        740      337      -403     
+ Partials      106       51       -55     
Impacted Files Coverage Δ
rocksdb/iterator.go 100.00% <100.00%> (ø)
remotedb/proto/defs.pb.go
Impacted Files Coverage Δ
rocksdb/iterator.go 100.00% <100.00%> (ø)
remotedb/proto/defs.pb.go

rocksdb/db.go Outdated Show resolved Hide resolved
Copy link
Contributor

@iproudhon iproudhon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@dudong2 dudong2 left a comment

Choose a reason for hiding this comment

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

LGTM

@egonspace egonspace merged commit 945e88f into main Dec 2, 2021
iproudhon pushed a commit that referenced this pull request Dec 2, 2021
* fix: should use copied value of iterator

* fix: apply comment
iproudhon added a commit that referenced this pull request Jan 21, 2022
* fix: should use copied value of iterator (#33)

* fix: should use copied value of iterator

* fix: apply comment

* feat: added Name method

* feat: alternative rocksdb implementation

* fix: fixed dummy Name() methods
feat: added WriteLowPri()

* chore: rdb instead of rocksdb for 'rocksdb' because gorocksdb doesn't have low priority write option

* chore: removed debug message

* fix: added cleveldb & rocksdb build for go test

* docs: update CHANGELOG.md

* fix: removed unused package

* fix: ci test failure

* Apply suggestions from code review

Co-authored-by: Youngtaek Yoon <noreply@yoon.anonaddy.me>

* fix: close upon batch write erroro

chore: removed duplicated line, added comments about read / write options

Co-authored-by: Woosang Son <woosang.son@linecorp.com>
Co-authored-by: Youngtaek Yoon <noreply@yoon.anonaddy.me>
@tnasu tnasu deleted the fix/copy_iter_value branch April 22, 2022 09:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants