You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jhump (via GitHub)" <gi...@apache.org> on 2025/04/09 15:30:16 UTC
[PR] Fix concurrency bug in REST catalog request signing [iceberg-go]
jhump opened a new pull request, #384:
URL: https://github.com/apache/iceberg-go/pull/384
A hasher is not thread-safe, yet the same hasher was being used for all requests. If applications made concurrent calls to the same REST catalog implementation, they could end up writing to the same hasher, corrupting the signatures for both concurrent requests.
This makes things thread-safe by creating a hasher for each signing operation. The could be safely re-used using a `sync.Pool. But the hasher is only 116 bytes, and initialization just has to write 8 bytes (other than the zero'ing done by the allocator), so it doesn't seem worth trying to re-use them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest): Fix concurrency bug in REST catalog request signing [iceberg-go]
Posted by "jhump (via GitHub)" <gi...@apache.org>.
jhump commented on PR #384:
URL: https://github.com/apache/iceberg-go/pull/384#issuecomment-2808046336
I've added a test. The test does not actually verify anything and passes, even if there's a concurrency bug, unless the race detector is enabled. With the race detector enabled, it did fail without the main fix in this PR:
```
==================
WARNING: DATA RACE
Read at 0x00c00119c168 by goroutine 54:
crypto/internal/fips140/sha256.(*Digest).Write()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/crypto/internal/fips140/sha256/sha256.go:163 +0x40
bytes.(*Reader).WriteTo()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/bytes/reader.go:143 +0xec
io.nopCloserWriterTo.WriteTo()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/io/io.go:702 +0xb8
io.(*nopCloserWriterTo).WriteTo()
<autogenerated>:1 +0x20
io.copyBuffer()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/io/io.go:411 +0xa8
io.Copy()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/io/io.go:388 +0x1f8
github.com/apache/iceberg-go/catalog/rest.(*sessionTransport).RoundTrip()
/Users/jhumphries/src/iceberg-go/catalog/rest/rest.go:227 +0x154
net/http.send()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/net/http/client.go:259 +0x66c
net/http.(*Client).send()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/net/http/client.go:180 +0x104
net/http.(*Client).do()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/net/http/client.go:728 +0xdd4
net/http.(*Client).Do()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/net/http/client.go:587 +0x1ac
github.com/apache/iceberg-go/catalog/rest.TestSigv4ConcurrentSigners.func3()
/Users/jhumphries/src/iceberg-go/catalog/rest/rest_internal_test.go:212 +0x190
golang.org/x/sync/errgroup.(*Group).Go.func1()
/Users/jhumphries/go/pkg/mod/golang.org/x/sync@v0.12.0/errgroup/errgroup.go:78 +0x7c
Previous write at 0x00c00119c168 by goroutine 50:
crypto/internal/fips140/sha256.(*Digest).Write()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/crypto/internal/fips140/sha256/sha256.go:163 +0x54
bytes.(*Reader).WriteTo()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/bytes/reader.go:143 +0xec
io.nopCloserWriterTo.WriteTo()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/io/io.go:702 +0xb8
io.(*nopCloserWriterTo).WriteTo()
<autogenerated>:1 +0x20
io.copyBuffer()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/io/io.go:411 +0xa8
io.Copy()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/io/io.go:388 +0x1f8
github.com/apache/iceberg-go/catalog/rest.(*sessionTransport).RoundTrip()
/Users/jhumphries/src/iceberg-go/catalog/rest/rest.go:227 +0x154
net/http.send()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/net/http/client.go:259 +0x66c
net/http.(*Client).send()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/net/http/client.go:180 +0x104
net/http.(*Client).do()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/net/http/client.go:728 +0xdd4
net/http.(*Client).Do()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/net/http/client.go:587 +0x1ac
github.com/apache/iceberg-go/catalog/rest.TestSigv4ConcurrentSigners.func3()
/Users/jhumphries/src/iceberg-go/catalog/rest/rest_internal_test.go:212 +0x190
golang.org/x/sync/errgroup.(*Group).Go.func1()
/Users/jhumphries/go/pkg/mod/golang.org/x/sync@v0.12.0/errgroup/errgroup.go:78 +0x7c
Goroutine 54 (running) created at:
golang.org/x/sync/errgroup.(*Group).Go()
/Users/jhumphries/go/pkg/mod/golang.org/x/sync@v0.12.0/errgroup/errgroup.go:75 +0x10c
github.com/apache/iceberg-go/catalog/rest.TestSigv4ConcurrentSigners()
/Users/jhumphries/src/iceberg-go/catalog/rest/rest_internal_test.go:197 +0x78c
testing.tRunner()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/testing/testing.go:1792 +0x180
testing.(*T).Run.gowrap1()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/testing/testing.go:1851 +0x40
Goroutine 50 (running) created at:
golang.org/x/sync/errgroup.(*Group).Go()
/Users/jhumphries/go/pkg/mod/golang.org/x/sync@v0.12.0/errgroup/errgroup.go:75 +0x10c
github.com/apache/iceberg-go/catalog/rest.TestSigv4ConcurrentSigners()
/Users/jhumphries/src/iceberg-go/catalog/rest/rest_internal_test.go:197 +0x78c
testing.tRunner()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/testing/testing.go:1792 +0x180
testing.(*T).Run.gowrap1()
/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.darwin-arm64/src/testing/testing.go:1851 +0x40
==================
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest): Fix concurrency bug in REST catalog request signing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #384:
URL: https://github.com/apache/iceberg-go/pull/384
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Fix concurrency bug in REST catalog request signing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #384:
URL: https://github.com/apache/iceberg-go/pull/384#issuecomment-2797500050
Is there any good way to test this that `-race` would find?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest): Fix concurrency bug in REST catalog request signing [iceberg-go]
Posted by "jhump (via GitHub)" <gi...@apache.org>.
jhump commented on PR #384:
URL: https://github.com/apache/iceberg-go/pull/384#issuecomment-2802360007
> Is there any good way to test this that `-race` would find?
I'll poke around. I think it will require being able to inject credentials (right now, the AWS package gets them from the environment when the signer is instantiated). But if we could provide them from a test harness, then we could add a test case that uses the sigv4 signing stuff and then have a few goroutines all using the same client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest): Fix concurrency bug in REST catalog request signing [iceberg-go]
Posted by "jhump (via GitHub)" <gi...@apache.org>.
jhump commented on code in PR #384:
URL: https://github.com/apache/iceberg-go/pull/384#discussion_r2045891208
##########
catalog/rest/rest.go:
##########
@@ -221,12 +222,12 @@ func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
return nil, err
}
- if _, err = io.Copy(s.h, rdr); err != nil {
+ h := s.newHash()
+ if _, err = io.Copy(h, rdr); err != nil {
return nil, err
}
- payloadHash = string(s.h.Sum(nil))
- s.h.Reset()
+ payloadHash = hex.EncodeToString(h.Sum(nil))
Review Comment:
Oops -- really need an integration test of some sort for this. Unfortunately, the AWS Go library doesn't seem to include a signature verifier that we can easily drop into a test server to make sure these are correct. But after stepping through the new test, I _think_ it should all work, with these fixes in.
##########
catalog/rest/rest.go:
##########
@@ -375,10 +376,7 @@ func handleNon200(rsp *http.Response, override map[int]error) error {
return e
}
-func fromProps(props iceberg.Properties) *options {
- o := &options{
- additionalProps: iceberg.Properties{},
- }
+func fromProps(props iceberg.Properties, o *options) {
Review Comment:
I changed the signature to make it less error-prone to round-trip the options into properties and back. This is done when creating a client, to convert options to properties, then merge in any properties retrieved from the server, and then convert back to options.
This way, we can easily clone the original options object and then override values from the properties. Previously, any time a new field was added to options that was not represented in properties (like the new `awsConfigSet` flag I added), if one forgot to update the list of manually copied fields ([here](https://github.com/apache/iceberg-go/pull/384/files#diff-b4f7aa1f4862334f1387eca97a977f5bcc7ba52a527562ad2f89cc4c767d618cL631-L632)), it would be easy to introduce a subtle bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest): Fix concurrency bug in REST catalog request signing [iceberg-go]
Posted by "jhump (via GitHub)" <gi...@apache.org>.
jhump commented on PR #384:
URL: https://github.com/apache/iceberg-go/pull/384#issuecomment-2808047190
> You should be able to inject credentials with the WithAwsConfig option for constructing the catalog, right?
Yep. Though this wasn't actually wired up. So I wired it up in 80b3f50beb372efc86cc670627b356b3fedec1ff.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest): Fix concurrency bug in REST catalog request signing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #384:
URL: https://github.com/apache/iceberg-go/pull/384#issuecomment-2808048353
Perfect! I'll take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(catalog/rest): Fix concurrency bug in REST catalog request signing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #384:
URL: https://github.com/apache/iceberg-go/pull/384#issuecomment-2803065824
You should be able to inject credentials with the `WithAwsConfig` option for constructing the catalog, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org