crypto/x509/pkix: Name.String overwrites ExtraNames backing store

[rsc]

CL 229864 added code to Name.String that looks like:

if len(n.ExtraNames) == 0 {
	for _, atv := range n.Names {
		...
		n.ExtraNames = append(n.ExtraNames, atv)
	}
}

ahead of the existing

return n.ToRDNSequence().String()

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

n.ExtraNames = nil // avoid writing on caller's slice backing store

just before the for loop.

A test of Name.String would also be good.
(I'm surprised this package has no tests at all.)

[cagedmantis]

/cc @FiloSottile

[g13013]

@cagedmantis does this issue require the Name.String test to be accepted ?

PS: I've never contributed to Golang and started yesterday

[g13013]

@cagedmantis @rsc
The CL contains the fix for this issue.

A test of Name.String would also be good.
(I'm surprised this package has no tests at all.)

It turned out that the tests for this package were under the x509 package in crypto/x509/x509_test.go file

should I move the tests for this package under crypto/x509/pkix as part of this issue ?

[gopherbot]

Change https://golang.org/cl/240317 mentions this issue: crypto/x509/pkix: fix Name.String overwriting ExtraNames

[g13013]

After a quick look, it seems that moving tests to pkix package would lead to duplication in test fixtures.

[gopherbot]

Change https://golang.org/cl/240543 mentions this issue: crypto/x509/pkix: print non-standard parsed Names at the end

[odeke-em]

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)
	}
}