Skip to content

Conversation

orisano
Copy link
Contributor

@orisano orisano commented Apr 25, 2022

fix #364

@orisano
Copy link
Contributor Author

orisano commented Apr 26, 2022

ping @goccy

@@ -386,6 +386,19 @@ func unescapeString(buf []byte) int {
v3 := hexToInt[char(src, 4)]
v4 := hexToInt[char(src, 5)]
code := rune((v1 << 12) | (v2 << 8) | (v3 << 4) | v4)
if code >= 0xd800 && code < 0xdc00 && uintptr(unsafeAdd(src, 11)) < uintptr(end) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここの少し上で 5 byte 先の char まで取得してますけど、 boundary check がなさそうなので中途半端な文字列が入っていると問題ありませんかね? ( \ufoo が入力文字列の最後みたいなとき )
同様に 11 byte 先を読む前に boundary check が必要そうです。

そして stream 処理側と処理を共通化したいと思って早1年...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

					case 'u':
						buflen := int64(len(buf))
						if cursor+5 >= buflen {
							return nil, 0, errors.ErrUnexpectedEndOfJSON("escaped string", cursor)
						}

ここでチェックしているので大丈夫だと思います

Copy link
Contributor Author

@orisano orisano Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同様に 11 byte 先を読む前に boundary check が必要そうです。

uintptr(unsafeAdd(src, 11)) < uintptr(end)
これが boundary check になっています

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

					case 'u':
						buflen := int64(len(buf))
						if cursor+5 >= buflen {
							return nil, 0, errors.ErrUnexpectedEndOfJSON("escaped string", cursor)
						}

このチェックが十分でないというのは以下のissueで上げています
#335

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほどそこに 5 bytes あるかのチェックがあったか。

uintptr(unsafeAdd(src, 11)) < uintptr(end)
これが boundary check になっています

これは 11 byte先の byte が終端文字かどうかを検査しているだけで、 buf が 11 byte先まで存在することを保証するものではないと思ったのですがどうでしょうか。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uintptr(end)がbufの末尾アドレスなのでsrc+11が末尾アドレスより小さければbufが11byte先まであると思います

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほど、 buf が heap にとられている前提があるから、address 値の大小を比較して boundary check しているんですね。 ( address 同士の比較じゃなくて中身を比較していると勘違いしていた )
この部分だけ boundary check の方法が他と違うのが気になりますが、とりあえず大丈夫そうです。

@goccy goccy merged commit 7719c4e into goccy:master Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emoticon unmarshall issue
2 participants