Skip to content

Commit 39dbb64

Browse files
tgeoghegantgeoghegan
authored
wfe: fix a race in orderForDisplay (#500)
I have an integration test for an ACME extension ([1]) that instantiate Pebble. I noticed that I would sometimes get test failures under `go test -race` like this: ``` ================== WARNING: DATA RACE Write at 0x00c00026cd80 by goroutine 143: github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).orderForDisplay.func2() /home/timg/source/pebble/wfe/wfe.go:1876 +0x255 math/rand.(*Rand).Shuffle() /usr/local/go/src/math/rand/rand.go:265 +0x96 math/rand.Shuffle() /usr/local/go/src/math/rand/rand.go:470 +0x38 github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).orderForDisplay() /home/timg/source/pebble/wfe/wfe.go:1875 +0x204 github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).Order() /home/timg/source/pebble/wfe/wfe.go:2021 +0x5ed github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).Order-fm() <autogenerated>:1 +0x69 github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).HandleFunc.func1() /home/timg/source/pebble/wfe/wfe.go:306 +0xa09 github.com/letsencrypt/pebble/v2/wfe.wfeHandlerFunc.ServeHTTP() /home/timg/source/pebble/wfe/wfe.go:146 +0x56 github.com/letsencrypt/pebble/v2/wfe.(*topHandler).ServeHTTP() /home/timg/source/pebble/wfe/wfe.go:158 +0x5b github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).HandleFunc.StripPrefix.func2() /usr/local/go/src/net/http/server.go:2282 +0x471 net/http.HandlerFunc.ServeHTTP() /usr/local/go/src/net/http/server.go:2220 +0x47 net/http.(*ServeMux).ServeHTTP() /usr/local/go/src/net/http/server.go:2747 +0x255 net/http.serverHandler.ServeHTTP() /usr/local/go/src/net/http/server.go:3210 +0x2a1 net/http.(*conn).serve() /usr/local/go/src/net/http/server.go:2092 +0x12a4 net/http.(*Server).Serve.gowrap3() /usr/local/go/src/net/http/server.go:3360 +0x4f Previous read at 0x00c00026cd80 by goroutine 274: github.com/letsencrypt/pebble/v2/ca.(*CAImpl).CompleteOrder() /home/timg/source/pebble/ca/ca.go:448 +0x76f github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).FinalizeOrder.func1() /home/timg/source/pebble/wfe/wfe.go:2215 +0x8d Goroutine 143 (running) created at: net/http.(*Server).Serve() /usr/local/go/src/net/http/server.go:3360 +0x8ec net/http.(*Server).ServeTLS() /usr/local/go/src/net/http/server.go:3401 +0x706 net/http.ServeTLS() /usr/local/go/src/net/http/server.go:2875 +0x269 github.com/tgeoghegan/oidf-box/test.setupPebble.func1() /home/timg/source/oidf-box/test/harness.go:297 +0x19c Goroutine 274 (running) created at: github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).FinalizeOrder() /home/timg/source/pebble/wfe/wfe.go:2214 +0x2fa4 github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).FinalizeOrder-fm() <autogenerated>:1 +0x69 github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).HandleFunc.func1() /home/timg/source/pebble/wfe/wfe.go:306 +0xa09 github.com/letsencrypt/pebble/v2/wfe.wfeHandlerFunc.ServeHTTP() /home/timg/source/pebble/wfe/wfe.go:146 +0x56 github.com/letsencrypt/pebble/v2/wfe.(*topHandler).ServeHTTP() /home/timg/source/pebble/wfe/wfe.go:158 +0x5b github.com/letsencrypt/pebble/v2/wfe.(*WebFrontEndImpl).HandleFunc.StripPrefix.func2() /usr/local/go/src/net/http/server.go:2282 +0x471 net/http.HandlerFunc.ServeHTTP() /usr/local/go/src/net/http/server.go:2220 +0x47 net/http.(*ServeMux).ServeHTTP() /usr/local/go/src/net/http/server.go:2747 +0x255 net/http.serverHandler.ServeHTTP() /usr/local/go/src/net/http/server.go:3210 +0x2a1 net/http.(*conn).serve() /usr/local/go/src/net/http/server.go:2092 +0x12a4 net/http.(*Server).Serve.gowrap3() /usr/local/go/src/net/http/server.go:3360 +0x4f ================== ``` What's happening is that `CAImpl.CompleteOrder` is fetching an `Order` from the "database" (which is in-memory) and iterating over the identifiers therein. `WebFrontEndImpl.orderForDisplay` fetches an `Order` from the database and then wants to construct a shuffled version of the order to return to the client to ensure nobody makes assumptions about the layout of these objects. `orderForDisplay` tries to take a copy of the `acme.Order`, but evidently that copies *pointers* to the `[]acme.Identifier Identifiers` and `[]string Authorizations` fields, which means shuffling them in the "copy" shuffles them in the DB object and makes the race detector unhappy. [1]: https://github.com/tgeoghegan/oidf-box
1 parent 3bb114e commit 39dbb64

File tree

1 file changed

+16
-2
lines changed

1 file changed

+16
-2
lines changed

‎wfe/wfe.go

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1840,8 +1840,22 @@ func (wfe *WebFrontEndImpl) orderForDisplay(
18401840
defer order.RUnlock()
18411841

18421842
// Copy the initial OrderRequest from the internal order object to mutate and
1843-
// use as the result.
1844-
result := order.Order
1843+
// use as the result. We have to make sure to copy the *contents* of the
1844+
// identifiers and authorizations slices and not the slices or the mutations
1845+
// below could mutate the order in the database, causing data races.
1846+
result := acme.Order{
1847+
Status: order.Order.Status,
1848+
Error: order.Order.Error,
1849+
Expires: order.Order.Expires,
1850+
Identifiers: slices.Clone(order.Order.Identifiers),
1851+
Profile: order.Order.Profile,
1852+
Finalize: order.Order.Finalize,
1853+
NotBefore: order.Order.NotBefore,
1854+
NotAfter: order.Order.NotAfter,
1855+
Authorizations: slices.Clone(order.Order.Authorizations),
1856+
Certificate: order.Order.Certificate,
1857+
Replaces: order.Order.Replaces,
1858+
}
18451859

18461860
// Randomize the order of the order authorization URLs as well as the order's
18471861
// identifiers. ACME draft Section 7.4 "Applying for Certificate Issuance"

0 commit comments

Comments
 (0)