-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix incorrect usage of json.NewDecoder #5107
Conversation
Unmarshal is faster and uses less memory anyway for small static values. This allowed invalid RUN commands in JSON format to pass when skipping bogus content and metadata commands to set the partial JSON value and skip the rest. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@@ -282,7 +282,7 @@ func parseJSON(rest string) (*Node, map[string]bool, error) { | |||
} | |||
|
|||
var myJSON []interface{} | |||
if err := json.NewDecoder(strings.NewReader(rest)).Decode(&myJSON); err != nil { | |||
if err := json.Unmarshal([]byte(rest), &myJSON); err != nil { |
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.
Contrary to moby/moby#12793 (comment) , json.Unmarshal
is actually faster and allocates significantly less memory than NewDecoder()
(always allocates at least 1KB) when parsing small JSON values.
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.
Perhaps worth updating some code in places then 😅
@@ -408,6 +408,9 @@ func (s3Client *s3Client) getManifest(ctx context.Context, key string, config *v | |||
if err := decoder.Decode(config); err != nil { | |||
return false, errors.WithStack(err) | |||
} | |||
if _, err := decoder.Token(); !errors.Is(err, io.EOF) { |
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.
Curious; I recall we used decoder.More()
to detect if there's additional content elsewhere. Would that work as well, or would that only look for valid JSON objects and ignore invalid content? https://github.com/moby/moby/blob/f3d377e4221903183faee08888da7d4bacf74450/api/server/httputils/httputils.go#L83
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.
More
doesn't seem to work correctly for some input characters https://go.dev/play/p/X4tmG7orx9W
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.
Ah! That's what I somewhat started to suspect when I saw the GoDoc mention "if there's another element". I wasn't sure if that would include invalid elements; thanks!
Looks like we need to update that code (and perhaps contribute a GoDoc improvement in Golang)
Fixes cases where
json.Decoder
was improperly used for parsing JSON values.Decode()
only parses the next object and does not check if there is additional content after it (unlikeUnmarshal()
).In Dockerfile it meant that currently invalid commands like this would cause no error and skip over the invalid bytes: