Fix dockerfile parser failing silently on long tokens#35429
Fix dockerfile parser failing silently on long tokens#35429thaJeztah merged 2 commits intomoby:masterfrom
Conversation
There was a problem hiding this comment.
For the sake of consistency, I suggest using bufio.MaxScanTokenSize instead.
builder/dockerfile/parser/parser.go
Outdated
There was a problem hiding this comment.
The error occurs when size is greater or equal.
a70fa48 to
8bae107
Compare
c0c7524 to
dd91b78
Compare
tonistiigi
left a comment
There was a problem hiding this comment.
Could you squash 2nd and 3rd commit
builder/remotecontext/remote.go
Outdated
There was a problem hiding this comment.
Would it be cleaner for this function just to take a ReadCloser and return ReadCloser so caller doesn't need to do any wrapping?
There was a problem hiding this comment.
I guess you mean move the ioutils.NewReadCloserWrapper() into inspectResponse() ? I think either way it needs to be wrapped, it's just about where it happens.
That could work. It would mean that the two four error returns in this function would have to call r.Close() instead of the one error case in the caller calling closer.
It also seems like it is more the responsibility of downloadRemote() since it is aware that it's a response body, where as inspectResponse only cares about a reader.
I have a very slight preference for the current implementation, but if you think it's cleaner to move it to inspectResponse I can do that.
I can also remove closer() that doesn't need to exist anymore, it can just be response.Body.Close
There was a problem hiding this comment.
ok, lets leave it like it is atm
Yes, definitely. Just wanted to make sure you were happy with the final result before squashing. Will do that now. |
Signed-off-by: Daniel Nephin <dnephin@docker.com>
The test was passing previously because the preamble was already buffered. After the change to return Scanner.Err() the final read error on the buffer was no longer being ignored. Signed-off-by: Daniel Nephin <dnephin@docker.com>
dd91b78 to
a74cc83
Compare
|
ping @tiborvass |
|
Ok to merge this? |
|
Ah, yes, I think so 👍 |
|
Is this bug elsewhere? I'm getting a null response on something like |
|
@disarticulate this fix is included in Docker 17.12 and up |
Fixes docker/for-linux#157
Fixes docker/for-mac#2208
Report errors when the scanner fails. The error message should include enough details for the user to fix the problem.
Fixing the first problem revealed another problem with handling remote urls that point at Dockerfiles, which is fixed in the second and third commits.