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

put function should copy Prefix #10

Open
emrekzd opened this issue Aug 6, 2014 · 3 comments
Open

put function should copy Prefix #10

emrekzd opened this issue Aug 6, 2014 · 3 comments

Comments

@emrekzd
Copy link

emrekzd commented Aug 6, 2014

Prefix is a named type of []byte, so passing it to put function (called by Set and Insert) passes a reference to the slice value. put should make a copy of the key:

    t := patricia.NewTrie()

    key := patricia.Prefix("foo")
    t.Insert(key, true)

    // alter the key slice
    key[0] = 'b'

    fmt.Println("boo key: ", t.Get(patricia.Prefix("boo")))
    fmt.Println("foo key: ", t.Get(patricia.Prefix("foo")))

Output:

    boo key:  true
    foo key:  <nil>

Sending a pull request for a fix.

@tchap
Copy link
Owner

tchap commented Aug 7, 2014

I am not sure we need to copy the prefix by default, perhaps it would be better to leave it like that and just document that the tree is taking over the ownership of the prefix that is passed there? @emrekzd

@emrekzd
Copy link
Author

emrekzd commented Aug 7, 2014

hello Ondrej. It is up to you but I don't agree with not copying it.

The issue is type casting a string to []byte causes the argument to be passed as a slice variable and gives the caller a way to modify trie keys without using the API. This becomes an issue when you pass a Prefix variable as argument.

It is more easy to see this if you think of the Visitor function func(p Prefix, i Item) error where Prefix is passed an argument. Because you get a reference to the prefix modifying it / inserting it into another trie will result in unexpected and counterintuitive behavior.

In fact it would be more convenient and safe to design the API to expect string types and have them converted to []byte internally, but since you want to keep the API as it is I think you should be making copies of the keys.

@tchap
Copy link
Owner

tchap commented Aug 7, 2014

@emrekzd Alright, I will think about it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants