Use io.ReadFull to read the bundle content #2433
Conversation
The io.Reader documentation says: > Read reads up to len(p) bytes into p. It returns the number of bytes > read (0 <= n <= len(p)) and any error encountered. ... If some data is > available but not len(p) bytes, Read conventionally returns what is > available instead of waiting for more. Read is not guaranteed to fill the data argument. Use io.ReadFull to fill the buffer. In some cases (a relatively big bundle), the bundle content was not completely read and it would fail to parse. Using `io.ReadFull` fixes the issue. *Note:* because bundles are cached, one will need to clean their cache bundle (`~/.tekton/bundles`) to make sure it re-fetches correct content. Signed-off-by: Vincent Demeester <[email protected]>
tekton-robot
added
release-note
tekton-robot
added
the
size/XS
|
cc @chmouel @piyush-garg @PuneetPunamiya |
|
This is related to tektoncd/pipeline#8388 |
| @@ -116,7 +116,7 @@ func readTarLayer(l v1.Layer) ([]byte, error) { | |||
| } | |||
|
|
|||
| contents := make([]byte, header.Size) | |||
| if _, err := treader.Read(contents); err != nil && err != io.EOF { | |||
| if _, err := io.ReadFull(treader, contents); err != nil && err != io.EOF { | |||
There was a problem hiding this comment.
that's not the same tho, you are going to read the whole content in memeory instead of stream it now.... would not cause other problems?
There was a problem hiding this comment.
Well today it doesn’t read everything. It truncates data. Also bundle are relatively small so it shouldn’t cause an issue in most case
There was a problem hiding this comment.
The extracted content would have to fit into an etcd, chunk? So there should be a maximum size of the blob, right?
If we want to prevent this from getting too big, we could potentially add validation into tkn bundle push to warn/fail if the size of the extracted content is too big.
There was a problem hiding this comment.
But yes, there is definitely a follow-up to be done here, but this does fixes some existing issue.
There was a problem hiding this comment.
Might want to have guard on header.Size not being over 1.5MB (https://etcd.io/docs/v3.5/dev-guide/limit/), might introduce a denial of service via crafted compressed layer (zip bomb)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arewm, vinamra28 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/lgtm |
Changes
The io.Reader documentation says:
Read is not guaranteed to fill the data argument. Use io.ReadFull to
fill the buffer.
In some cases (a relatively big bundle), the bundle content was not
completely read and it would fail to parse. Using
io.ReadFullfixesthe issue.
Note: because bundles are cached, one will need to clean their cache
bundle (
~/.tekton/bundles) to make sure it re-fetches correctcontent.
Signed-off-by: Vincent Demeester [email protected]
/kind bug
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make checkmake generatedSee the contribution guide
for more details.
Release Notes