From 5ea0aea5e514a1fb1b63c11da2c37a6c0ec7df61 Mon Sep 17 00:00:00 2001 From: Tim Geoghegan Date: Tue, 20 May 2025 10:03:04 -0700 Subject: [PATCH 1/3] wfe: fix a race in `orderForDisplay` 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() :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() :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. Explicit `slices.Clone` calls make sure we're not mutating objects from the database. [1]: https://github.com/tgeoghegan/oidf-box --- wfe/wfe.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index 142bf804..9b12c5bd 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1840,8 +1840,12 @@ func (wfe *WebFrontEndImpl) orderForDisplay( defer order.RUnlock() // Copy the initial OrderRequest from the internal order object to mutate and - // use as the result. + // use as the result. We have to make sure to copy the identifiers and + // authorizations *slices* and not *pointers to the slices* or the mutations + // below could mutate the order in the database, causing data races. result := order.Order + result.Identifiers = slices.Clone(order.Order.Identifiers) + result.Authorizations = slices.Clone(order.Order.Authorizations) // Randomize the order of the order authorization URLs as well as the order's // identifiers. ACME draft Section 7.4 "Applying for Certificate Issuance" From 27d9c1756f6bd0e874546c6428149aab2b123d25 Mon Sep 17 00:00:00 2001 From: Tim Geoghegan Date: Tue, 20 May 2025 15:12:25 -0700 Subject: [PATCH 2/3] explicitly construct copied order --- wfe/wfe.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index 9b12c5bd..d2feae75 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1843,9 +1843,19 @@ func (wfe *WebFrontEndImpl) orderForDisplay( // use as the result. We have to make sure to copy the identifiers and // authorizations *slices* and not *pointers to the slices* or the mutations // below could mutate the order in the database, causing data races. - result := order.Order - result.Identifiers = slices.Clone(order.Order.Identifiers) - result.Authorizations = slices.Clone(order.Order.Authorizations) + result := acme.Order{ + Status: order.Order.Status, + Error: order.Order.Error, + Expires: order.Order.Expires, + Identifiers: slices.Clone(order.Order.Identifiers), + Profile: order.Order.Profile, + Finalize: order.Order.Finalize, + NotBefore: order.Order.NotBefore, + NotAfter: order.Order.NotAfter, + Authorizations: slices.Clone(order.Order.Authorizations), + Certificate: order.Order.Certificate, + Replaces: order.Order.Replaces, + } // Randomize the order of the order authorization URLs as well as the order's // identifiers. ACME draft Section 7.4 "Applying for Certificate Issuance" From ad0cd4830eed5116151955a1fb19fe5bde4793cd Mon Sep 17 00:00:00 2001 From: Tim Geoghegan Date: Wed, 21 May 2025 08:14:40 -0700 Subject: [PATCH 3/3] improve comment --- wfe/wfe.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index d2feae75..2b363bb4 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1840,8 +1840,8 @@ func (wfe *WebFrontEndImpl) orderForDisplay( defer order.RUnlock() // Copy the initial OrderRequest from the internal order object to mutate and - // use as the result. We have to make sure to copy the identifiers and - // authorizations *slices* and not *pointers to the slices* or the mutations + // use as the result. We have to make sure to copy the *contents* of the + // identifiers and authorizations slices and not the slices or the mutations // below could mutate the order in the database, causing data races. result := acme.Order{ Status: order.Order.Status,