Skip to content

[1.3] bucket.Put: copy key before seek#639

Merged
ahrtr merged 5 commits intoetcd-io:release-1.3from
fuweid:cp-copy-key-before-seek
Dec 18, 2023
Merged

[1.3] bucket.Put: copy key before seek#639
ahrtr merged 5 commits intoetcd-io:release-1.3from
fuweid:cp-copy-key-before-seek

Conversation

@fuweid
Copy link
Copy Markdown
Member

@fuweid fuweid commented Dec 13, 2023

Comment thread bucket.go
return ErrValueTooLarge
}

// Insert into node.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's related to #550.

If it's not accepted, I can use key here. Or I can cherry-pick 550 first.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please backport both #550 and #637. They are highly related, so let's just backport both of them in this PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated

As per `go build -gcflags -m ./... 2>&1`:

Old behaviour:
```
./bucket.go:148:31: leaking param: key
./bucket.go:192:42: leaking param: key
./bucket.go:271:22: leaking param: key
```

Now:
```
./bucket.go:148:31: key does not escape
./bucket.go:192:42: key does not escape
./bucket.go:271:22: key does not escape
```

Signed-off-by: Evgenii Stratonikov <fyfyrchik@runbox.com>
(cherry picked from commit 71a59ca)
Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid force-pushed the cp-copy-key-before-seek branch from 581c892 to 31b5a34 Compare December 14, 2023 00:45
Comment thread bucket.go Outdated
c.node().put(key, key, value, 0, bucketLeafFlag)
// Tip: Use a new variable `newKey` instead of reusing the existing `key` to prevent
// it from being marked as leaking, and accordingly cannot be allocated on stack.
newKey := cloneBytes(key)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should clone the key before seek (line 167). Please update the main branch firstly. Thanks.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
(cherry picked from commit 324df9c)
Signed-off-by: Wei Fu <fuweid89@gmail.com>
Signed-off-by: Wei Fu <fuweid89@gmail.com>
(cherry picked from commit 1b08078)
Signed-off-by: Wei Fu <fuweid89@gmail.com>
Application might change key value after seeking and before real put.
This unexpected behaviour could corrupt database. When users file issue,
maintainers doesn't know application behaviour. It could be caused by
data race. This patch is to prevent such case and save maintainers' time.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
(cherry picked from commit a05ec68)
Signed-off-by: Wei Fu <fuweid89@gmail.com>
It's follow-up of etcd-io#637.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
(cherry picked from commit 62d8026)
Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid force-pushed the cp-copy-key-before-seek branch from 31b5a34 to fabe2fb Compare December 18, 2023 15:39
@fuweid fuweid marked this pull request as ready for review December 18, 2023 15:48
Copy link
Copy Markdown
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks

@ahrtr ahrtr merged commit e102fcf into etcd-io:release-1.3 Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants