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.

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

Illegal slice reuse in Badger code #331

Open
boreq opened this issue Feb 13, 2023 · 2 comments
Open

Illegal slice reuse in Badger code #331

boreq opened this issue Feb 13, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@boreq
Copy link
Contributor

boreq commented Feb 13, 2023

In my opinion code such as this is incorrect:

go-ssb/graph/builder.go

Lines 105 to 112 in 2cdd828

for iter.Seek(prefix); iter.ValidForPrefix(prefix); iter.Next() {
it := iter.Item()
k := it.Key()
if err := txn.Delete(k); err != nil {
return fmt.Errorf("DeleteAuthor: failed to drop record %x: %w", k, err)
}
}

Reasoning:

https://pkg.go.dev/github.com/dgraph-io/badger/v3#Item.Key:

Key is only valid as long as item is valid, or transaction is valid. If you need to use it outside its validity, please use KeyCopy.

https://pkg.go.dev/github.com/dgraph-io/badger/v3#Iterator.Item:

Item returns pointer to the current key-value pair. This item is only valid until it.Next() gets called.

In my opinion in this situation the item is only valid until the next iteration therefore key is illegaly reused. KeyCopy should be used instead. It is possible that txn.Delete(...) will remove other data. I imagine this is the case in other places in the code as well.

@decentral1se
Copy link
Member

decentral1se commented Feb 14, 2023

Nice digging, yeh, that seems sensible to change.

Not many references to Key() here, maybe more elsewhere in the dependencies?

grep -R "it.Key()" . -l
./graph/builder.go
./plugins2/names/about.go

@decentral1se decentral1se added the bug Something isn't working label Feb 14, 2023
@boreq
Copy link
Contributor Author

boreq commented Feb 15, 2023

Method
    Key
Usages in All Places  (7 usages found)
    Unclassified  (7 usages found)
        go-ssb  (6 usages found)
            graph  (4 usages found)
                builder.go  (4 usages found)
                    Build  (1 usage found)
                        143 k := it.Key()
                    DeleteAuthor  (1 usage found)
                        108 k := it.Key()
                    Follows  (1 usage found)
                        265 k := it.Key()
                    Subfeeds  (1 usage found)
                        339 k := it.Key()
            plugins2/names  (2 usages found)
                about.go  (2 usages found)
                    All  (1 usage found)
                        99 k := it.Key()
                    CollectedFor  (1 usage found)
                        163 k := it.Key()
        /home/filip/go/pkg/mod/github.com/ssbc/[email protected]/internal/persist/badger  (1 usage found)
            saver.go  (1 usage found)
                List  (1 usage found)
                    107 k := it.Key()

I can see that the key is being taken out of the transaction and stored in a slice in margaret. If I am correct about this then this could be a source of really weird bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants