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
crypto/x509/pkix: Name.String overwrites ExtraNames backing store #39873
Comments
/cc @FiloSottile |
@cagedmantis does this issue require the PS: I've never contributed to Golang and started yesterday |
@cagedmantis @rsc
It turned out that the tests for this package were under the should I move the tests for this package under crypto/x509/pkix as part of this issue ? |
Change https://golang.org/cl/240317 mentions this issue: |
After a quick look, it seems that moving tests to |
Change https://golang.org/cl/240543 mentions this issue: |
Here is a test for @rsc's mentioned bug, that we can perhaps use in the fix to prevent regressions package pkix_test
import (
"encoding/asn1"
"reflect"
"testing"
"crypto/x509/pkix"
)
// Issue 39873: Ensure that invoking Name.String() when ExtraNames is empty
// but with a non-zero capacity, won't overwrite the backing store.
func TestNameDotStringDoesnotOverwriteBackingSlice(t *testing.T) {
backing := []pkix.AttributeTypeAndValue{
{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "original.org"},
}
n := &pkix.Name{
Locality: []string{"Gophertown"},
Names: []pkix.AttributeTypeAndValue{
{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "tbd.org"},
},
ExtraNames: backing[:0],
}
if g, w := n.String(), "1.2.3.4.5=#13077462642e6f7267,L=Gophertown"; g != w {
t.Errorf(".String mismatch\nGot: %q\nWant: %q", g, w)
}
wantExtraNames := []pkix.AttributeTypeAndValue{
{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "example.org"},
}
if false && !reflect.DeepEqual(n.ExtraNames, wantExtraNames) {
t.Fatalf("ExtraNames mismatch\nGot: %+v\nWant: %+v\n", n.ExtraNames, wantExtraNames)
}
wantBacking := []pkix.AttributeTypeAndValue{
{Type: asn1.ObjectIdentifier([]int{1, 2, 3, 4, 5}), Value: "original.org"},
}
if !reflect.DeepEqual(backing, wantBacking) {
t.Fatalf("Backing mismatch\nGot: %+v\nWant: %+v\n", backing, wantBacking)
}
} |
CL 229864 added code to Name.String that looks like:
ahead of the existing
This code has a subtle bug: if n.ExtraNames has len 0 but non-zero cap, this loop scribbles over n.ExtraName's backing store. By convention, String methods don't mutate the receiver, but this one does.
The code should add
just before the for loop.
A test of Name.String would also be good.
(I'm surprised this package has no tests at all.)
The text was updated successfully, but these errors were encountered: