ssbc / go-ssb

Go implementation of ssb (work in progress!)
https://scuttlebutt.nz
160 stars 26 forks source link

Illegal slice reuse in Badger code #331

Open boreq opened 1 year ago

boreq commented 1 year ago

In my opinion code such as this is incorrect: https://github.com/ssbc/go-ssb/blob/2cdd828cd8c898441c18b4b05790dc17007519ee/graph/builder.go#L105-L112

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 commented 1 year ago

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
boreq commented 1 year ago
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/margaret@v0.4.4-0.20221101112304-4f5815095ef3/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.