[インデックス 10483] ファイルの概要

このコミットは、Go言語の標準ライブラリである encoding/xml パッケージにおけるコピーバグを修正するものです。具体的には、StartElement 型の Copy メソッドにおいて、属性（Attr）のスライスを正しくコピーできていなかった問題に対処しています。これにより、元の StartElement の属性を変更すると、コピーされた StartElement の属性も意図せず変更されてしまうという、いわゆる「シャローコピー」の問題が発生していました。

コミット

commit fe838c2ddb89014202299f9ab95685097753784e
Author: Russ Cox <rsc@golang.org>
Date:   Tue Nov 22 12:31:33 2011 -0500

    encoding/xml: fix copy bug
    
    Fixes #2484.
    
    R=golang-dev, bradfitz
    CC=golang-dev
    https://golang.org/cl/5417059

GitHub上でのコミットページへのリンク

https://github.com/golang/go/commit/fe838c2ddb89014202299f9ab95685097753784e

元コミット内容

このコミットは、encoding/xml パッケージ内のコピーバグを修正します。具体的には、StartElement 型の Copy メソッドが、その内部のスライスである Attr を正しくコピーしていなかったため、元の StartElement の Attr を変更すると、コピーされた StartElement の Attr も変更されてしまうという問題（Issue 2484）を解決します。

変更の背景

Go言語の encoding/xml パッケージは、XMLドキュメントのエンコードとデコードを扱うための標準ライブラリです。XMLドキュメントをパースする際、StartElement はXML要素の開始タグを表し、その属性（Attr）を保持します。

StartElement 型には Copy() メソッドが定義されており、これは StartElement のディープコピーを作成することを意図していました。しかし、元の実装では、Attr スライスをコピーする際に copy(e.Attr, attrs) と記述されていました。Go言語の copy 関数は copy(dst, src) の形式で、src の内容を dst にコピーします。この場合、e.Attr がコピー元、attrs がコピー先として指定されていましたが、attrs は新しく作成された空のスライスであり、e.Attr の内容が attrs にコピーされるのではなく、attrs の内容（空）が e.Attr にコピーされるという誤った動作をしていました。

結果として、e.Attr は変更されず、新しく作成された attrs スライスも空のままでした。その後、e.Attr = attrs とすることで、e.Attr は空のスライスを指すことになり、元の StartElement の属性情報が失われるか、あるいは e.Attr が元のスライスを指したままとなり、ディープコピーが正しく行われないという問題が発生していました。

このバグは、Issue 2484として報告されており、StartElement のコピーが期待通りに機能しないことで、XML処理ロジックに予期せぬ副作用をもたらす可能性がありました。

前提知識の解説

Go言語のスライスとコピー

Go言語のスライスは、配列への参照のようなものです。スライス自体は、基底となる配列へのポインタ、長さ、容量の3つの要素から構成されます。スライスを別のスライスに代入すると、同じ基底配列を共有する新しいスライスが作成されます（シャローコピー）。

ディープコピーを行うには、新しい基底配列を作成し、元のスライスの要素を新しい配列にコピーする必要があります。Go言語の組み込み関数 copy(dst, src []Type) は、src スライスの要素を dst スライスにコピーします。この関数は、コピーされた要素の数を返します。重要なのは、copy 関数は dst スライスが指す基底配列に要素を書き込むという点です。

`encoding/xml` パッケージ

encoding/xml パッケージは、GoプログラムとXMLドキュメントの間でデータを変換するための機能を提供します。主な機能は以下の通りです。

XMLのパース（デコード）: XMLドキュメントをGoのデータ構造に変換します。
XMLの生成（エンコード）: Goのデータ構造をXMLドキュメントに変換します。

このパッケージでは、XMLの要素や属性を表現するための様々な型が定義されています。

StartElement: XMLの開始タグ（例: <tag attr="value">）を表します。Name フィールドと Attr フィールドを持ちます。
Attr: XMLの属性（例: attr="value"）を表します。Name フィールドと Value フィールドを持ちます。
Name: XMLの名前空間とローカル名を組み合わせた名前を表します。

`reflect.DeepEqual`

reflect.DeepEqual は、Go言語の reflect パッケージに含まれる関数で、2つの値が「深く」等しいかどうかを判定します。これは、プリミティブ型だけでなく、構造体、配列、スライス、マップなどの複合型についても、その内容が再帰的に等しいかを比較します。このコミットのテストコードでは、コピーが正しく行われたかどうかを検証するために使用されています。

技術的詳細

誤った `copy` 関数の使用

元のコードでは、StartElement.Copy() メソッド内で以下のように記述されていました。

func (e StartElement) Copy() StartElement {
	attrs := make([]Attr, len(e.Attr))
	copy(e.Attr, attrs) // ここが問題
	e.Attr = attrs
	return e
}

ここで問題となるのは copy(e.Attr, attrs) の行です。Go言語の copy 関数は copy(dst, src) の形式で、src スライスの内容を dst スライスにコピーします。この場合、e.Attr が src（コピー元）として、attrs が dst（コピー先）として指定されるべきでした。しかし、誤って e.Attr が dst、attrs が src として指定されていました。

attrs は make([]Attr, len(e.Attr)) によって作成された新しいスライスであり、その要素はゼロ値（Attr 型のゼロ値）で初期化されています。したがって、copy(e.Attr, attrs) は、attrs の空の（ゼロ値の）内容を e.Attr にコピーしようとします。しかし、e.Attr は元の StartElement の属性スライスであり、この操作は e.Attr の内容を上書きするものではありません。実際には、attrs の長さが e.Attr の長さよりも短いため、何もコピーされません。

その後の e.Attr = attrs の行で、e.Attr は新しく作成された空の attrs スライスを指すようになります。これにより、元の StartElement の属性情報が失われ、ディープコピーが正しく行われないという結果になっていました。

修正内容

修正後のコードは以下のようになります。

func (e StartElement) Copy() StartElement {
	attrs := make([]Attr, len(e.Attr))
	copy(attrs, e.Attr) // 修正箇所
	e.Attr = attrs
	return e
}

この修正では、copy(attrs, e.Attr) とすることで、e.Attr（元の属性スライス）の内容が attrs（新しく作成されたスライス）に正しくコピーされるようになりました。これにより、attrs は元の StartElement の属性のディープコピーを保持し、その後の e.Attr = attrs によって、コピーされた StartElement がこの新しいディープコピーされた属性スライスを指すようになります。

テストコードの変更

テストコード src/pkg/encoding/xml/xml_test.go も、このバグをより正確に検出できるように変更されました。

元のテストでは、tok1 と tok2 がディープコピーされていることを reflect.DeepEqual で確認した後、elt.Attr[0] を変更して、tok1 と tok2 が異なることを確認していました。しかし、elt は tok1 の基底となる StartElement であり、tok1 は elt の値コピーです。そのため、elt.Attr[0] を変更しても、tok1 の Attr スライスには影響しませんでした。

修正後のテストでは、tok1.(StartElement).Attr[0] を変更するように変更されました。これにより、tok1 が指す StartElement の属性が変更され、もし tok2 がシャローコピーであれば tok2 の属性も変更されてしまうため、ディープコピーが正しく行われているかをより厳密に検証できるようになりました。

また、CopyToken が Attr[0] を上書きしていないことを確認するための新しいアサーションも追加されました。これは、CopyToken の呼び出し後に tok1.(StartElement).Attr[0].Value がまだ "en" であることを確認することで、コピー操作が元のデータを破壊していないことを保証します。

コアとなるコードの変更箇所

`src/pkg/encoding/xml/xml.go`

--- a/src/pkg/encoding/xml/xml.go
+++ b/src/pkg/encoding/xml/xml.go
@@ -61,7 +61,7 @@ type StartElement struct {
 
 func (e StartElement) Copy() StartElement {
 	attrs := make([]Attr, len(e.Attr))
-	copy(e.Attr, attrs)
+	copy(attrs, e.Attr)
 	e.Attr = attrs
 	return e
 }

`src/pkg/encoding/xml/xml_test.go`

--- a/src/pkg/encoding/xml/xml_test.go
+++ b/src/pkg/encoding/xml/xml_test.go
@@ -486,10 +486,13 @@ func TestCopyTokenStartElement(t *testing.T) {
 	elt := StartElement{Name{"", "hello"}, []Attr{{Name{"", "lang"}, "en"}}}
 	var tok1 Token = elt
 	tok2 := CopyToken(tok1)
+	if tok1.(StartElement).Attr[0].Value != "en" {
+		t.Error("CopyToken overwrote Attr[0]")
+	}
 	if !reflect.DeepEqual(tok1, tok2) {
 		t.Error("CopyToken(StartElement) != StartElement")
 	}
-	elt.Attr[0] = Attr{Name{"", "lang"}, "de"}
+	tok1.(StartElement).Attr[0] = Attr{Name{"", "lang"}, "de"}
 	if reflect.DeepEqual(tok1, tok2) {
 		t.Error("CopyToken(CharData) uses same buffer.")
 	}

コアとなるコードの解説

`src/pkg/encoding/xml/xml.go` の変更

StartElement 型の Copy() メソッドは、StartElement のディープコピーを生成することを目的としています。

変更前: copy(e.Attr, attrs)
- これは copy(dst, src) の形式で、dst に e.Attr（元のスライス）、src に attrs（新しく作成された空のスライス）が指定されていました。
- 結果として、attrs の空の内容が e.Attr にコピーされようとしましたが、attrs の長さが e.Attr の長さよりも短いため、実質的に何もコピーされませんでした。
- その後、e.Attr = attrs で e.Attr が新しく作成された空のスライスを指すようになり、元の属性情報が失われるか、ディープコピーが失敗していました。
変更後: copy(attrs, e.Attr)
- これは copy(dst, src) の正しい形式で、dst に attrs（新しく作成されたスライス）、src に e.Attr（元のスライス）が指定されています。
- これにより、e.Attr の内容が attrs に正しくコピーされ、attrs は元の属性スライスのディープコピーとなります。
- その後の e.Attr = attrs によって、コピーされた StartElement は、このディープコピーされた属性スライスを指すようになり、期待通りのディープコピーが実現されます。

`src/pkg/encoding/xml/xml_test.go` の変更

TestCopyTokenStartElement 関数は、StartElement の Copy() メソッドが正しく機能するかをテストします。

追加された行:
```
if tok1.(StartElement).Attr[0].Value != "en" {
	t.Error("CopyToken overwrote Attr[0]")
}
```
- これは、CopyToken（内部で StartElement.Copy() を呼び出す）が実行された後も、元の tok1 の属性値が変更されていないことを確認するためのアサーションです。これにより、コピー操作が元のデータを誤って上書きしていないことを保証します。
変更された行:
```
-	elt.Attr[0] = Attr{Name{"", "lang"}, "de"}
+	tok1.(StartElement).Attr[0] = Attr{Name{"", "lang"}, "de"}
```
- 変更前は、elt（tok1 の基底となる StartElement）の属性を変更していました。しかし、tok1 は elt の値コピーであるため、elt の変更は tok1 には影響しませんでした。
- 変更後は、tok1.(StartElement).Attr[0] を直接変更しています。これにより、tok1 が指す StartElement の属性が変更されます。
- この変更により、reflect.DeepEqual(tok1, tok2) のチェックがより効果的になります。もし tok2 が tok1 のシャローコピーであれば、tok1 の属性変更は tok2 にも影響し、reflect.DeepEqual は true を返してテストが失敗します。これにより、Copy() メソッドが真のディープコピーを行っていることを厳密に検証できます。

参考にした情報源リンク

Go言語の copy 関数に関するドキュメント: https://pkg.go.dev/builtin#copy
Go言語の Issue 2484: encoding/xml: fix copy bug (このコミットが修正したIssue) - GitHubのコミットメッセージに記載されている Fixes #2484 から検索可能。
- https://github.com/golang/go/issues/2484
Go言語のコードレビューシステム (Gerrit) の変更リスト: https://golang.org/cl/5417059
- https://go-review.googlesource.com/c/go/+/5417059
- このリンクは、コミットメッセージに記載されているGerritの変更リストへのリンクです。通常、より詳細な議論やレビューコメントが含まれています。

[インデックス 10483] ファイルの概要

コミット

commit fe838c2ddb89014202299f9ab95685097753784e
Author: Russ Cox <rsc@golang.org>
Date:   Tue Nov 22 12:31:33 2011 -0500

    encoding/xml: fix copy bug
    
    Fixes #2484.
    
    R=golang-dev, bradfitz
    CC=golang-dev
    https://golang.org/cl/5417059

GitHub上でのコミットページへのリンク

https://github.com/golang/go/commit/fe838c2ddb89014202299f9ab95685097753784e

元コミット内容

変更の背景

StartElement 型には Copy() メソッドが定義されており、これは StartElement のディープコピーを作成することを意図していました。しかし、元の実装では、Attr スライスをコピーする際に copy(e.Attr, attrs) と記述されていました。Go言語の copy 関数は copy(dst, src) の形式で、src の内容を dst にコピーします。この場合、e.Attr がコピー元、attrs がコピー先として指定されていましたが、誤って e.Attr が dst、attrs が src として指定されていました。

結果として、attrs は make([]Attr, len(e.Attr)) によって作成された新しいスライスであり、その要素はゼロ値（Attr 型のゼロ値）で初期化されています。したがって、copy(e.Attr, attrs) は、attrs の空の（ゼロ値の）内容を e.Attr にコピーしようとします。しかし、e.Attr は元の StartElement の属性スライスであり、この操作は e.Attr の内容を上書きするものではありません。実際には、attrs の長さが e.Attr の長さよりも短いため、何もコピーされませんでした。

その後の e.Attr = attrs の行で、e.Attr は新しく作成された空の attrs スライスを指すことになり、元の StartElement の属性情報が失われるか、あるいは e.Attr が元のスライスを指したままとなり、ディープコピーが正しく行われないという問題が発生していました。

XMLのパース（デコード）: XMLドキュメントをGoのデータ構造に変換します。
XMLの生成（エンコード）: Goのデータ構造をXMLドキュメントに変換します。

このパッケージでは、XMLの要素や属性を表現するための様々な型が定義されています。

StartElement: XMLの開始タグ（例: <tag attr="value">）を表します。Name フィールドと Attr フィールドを持ちます。
Attr: XMLの属性（例: attr="value"）を表します。Name フィールドと Value フィールドを持ちます。
Name: XMLの名前空間とローカル名を組み合わせた名前を表します。

func (e StartElement) Copy() StartElement {
	attrs := make([]Attr, len(e.Attr))
	copy(e.Attr, attrs) // ここが問題
	e.Attr = attrs
	return e
}

修正内容

修正後のコードは以下のようになります。

func (e StartElement) Copy() StartElement {
	attrs := make([]Attr, len(e.Attr))
	copy(attrs, e.Attr) // 修正箇所
	e.Attr = attrs
	return e
}

--- a/src/pkg/encoding/xml/xml.go
+++ b/src/pkg/encoding/xml/xml.go
@@ -61,7 +61,7 @@ type StartElement struct {
 
 func (e StartElement) Copy() StartElement {
 	attrs := make([]Attr, len(e.Attr))
-	copy(e.Attr, attrs)
+	copy(attrs, e.Attr)
 	e.Attr = attrs
 	return e
 }

`src/pkg/encoding/xml/xml_test.go`

--- a/src/pkg/encoding/xml/xml_test.go
+++ b/src/pkg/encoding/xml/xml_test.go
@@ -486,10 +486,13 @@ func TestCopyTokenStartElement(t *testing.T) {
 	elt := StartElement{Name{"", "hello"}, []Attr{{Name{"", "lang"}, "en"}}}
 	var tok1 Token = elt
 	tok2 := CopyToken(tok1)
+	if tok1.(StartElement).Attr[0].Value != "en" {
+		t.Error("CopyToken overwrote Attr[0]")
+	}
 	if !reflect.DeepEqual(tok1, tok2) {
 		t.Error("CopyToken(StartElement) != StartElement")
 	}
-	elt.Attr[0] = Attr{Name{"", "lang"}, "de"}
+	tok1.(StartElement).Attr[0] = Attr{Name{"", "lang"}, "de"}
 	if reflect.DeepEqual(tok1, tok2) {
 		t.Error("CopyToken(CharData) uses same buffer.")
 	}

コアとなるコードの解説

`src/pkg/encoding/xml/xml.go` の変更

StartElement 型の Copy() メソッドは、StartElement のディープコピーを生成することを目的としています。

変更前: copy(e.Attr, attrs)
- これは copy(dst, src) の形式で、dst に e.Attr（元のスライス）、src に attrs（新しく作成された空のスライス）が指定されていました。
- 結果として、attrs の空の内容が e.Attr にコピーされようとしましたが、attrs の長さが e.Attr の長さよりも短いため、実質的に何もコピーされませんでした。
- その後、e.Attr = attrs で e.Attr が新しく作成された空のスライスを指すようになり、元の属性情報が失われるか、ディープコピーが失敗していました。
変更後: copy(attrs, e.Attr)
- これは copy(dst, src) の正しい形式で、dst に attrs（新しく作成されたスライス）、src に e.Attr（元のスライス）が指定されています。
- これにより、e.Attr の内容が attrs に正しくコピーされ、attrs は元の属性スライスのディープコピーとなります。
- その後の e.Attr = attrs によって、コピーされた StartElement は、このディープコピーされた属性スライスを指すようになり、期待通りのディープコピーが実現されます。

`src/pkg/encoding/xml/xml_test.go` の変更

TestCopyTokenStartElement 関数は、StartElement の Copy() メソッドが正しく機能するかをテストします。

追加された行:
```
if tok1.(StartElement).Attr[0].Value != "en" {
	t.Error("CopyToken overwrote Attr[0]")
}
```
- これは、CopyToken（内部で StartElement.Copy() を呼び出す）が実行された後も、元の tok1 の属性値が変更されていないことを確認するためのアサーションです。これにより、コピー操作が元のデータを誤って上書きしていないことを保証します。
変更された行:
```
-	elt.Attr[0] = Attr{Name{"", "lang"}, "de"}
+	tok1.(StartElement).Attr[0] = Attr{Name{"", "lang"}, "de"}
```
- 変更前は、elt（tok1 の基底となる StartElement）の属性を変更していました。しかし、tok1 は elt の値コピーであるため、elt の変更は tok1 には影響しませんでした。
- 変更後は、tok1.(StartElement).Attr[0] を直接変更しています。これにより、tok1 が指す StartElement の属性が変更されます。
- この変更により、reflect.DeepEqual(tok1, tok2) のチェックがより効果的になります。もし tok2 が tok1 のシャローコピーであれば、tok1 の属性変更は tok2 にも影響し、reflect.DeepEqual は true を返してテストが失敗します。これにより、Copy() メソッドが真のディープコピーを行っていることを厳密に検証できます。

参考にした情報源リンク

Go言語の copy 関数に関するドキュメント: https://pkg.go.dev/builtin#copy
Go言語の Issue 2484: encoding/xml: fix copy bug (このコミットが修正したIssue) - GitHubのコミットメッセージに記載されている Fixes #2484 から検索可能。
- https://github.com/golang/go/issues/2484
Go言語のコードレビューシステム (Gerrit) の変更リスト: https://golang.org/cl/5417059
- https://go-review.googlesource.com/c/go/+/5417059
- このリンクは、コミットメッセージに記載されているGerritの変更リストへのリンクです。通常、より詳細な議論やレビューコメントが含まれています。

Keyboard shortcuts

comemo