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 <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
| @@ -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.
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.
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.
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.
More doesn't seem to work correctly for some input characters https://go.dev/play/p/X4tmG7orx9W
There was a problem hiding this comment.
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.Decoderwas 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: