-
-
Notifications
You must be signed in to change notification settings - Fork 175
fix: to care surrogate-pair on stringDecoder #365
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
Conversation
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) { |
There was a problem hiding this comment.
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年...
There was a problem hiding this comment.
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)
}
ここでチェックしているので大丈夫だと思います
There was a problem hiding this comment.
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 になっています
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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先まで存在することを保証するものではないと思ったのですがどうでしょうか。
There was a problem hiding this comment.
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先まであると思います
There was a problem hiding this comment.
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 の方法が他と違うのが気になりますが、とりあえず大丈夫そうです。
fix #364