vendor: add local copy of archive/tar#40672
Conversation
There was a problem hiding this comment.
I'm guessing this description isn't accurate given we're patching archive/tar again in a post-1.11 Go version?
There was a problem hiding this comment.
We reused the same commit that we used in the past (but at the time for a different reason)
There was a problem hiding this comment.
Right, that makes sense, but shouldn't we update the description? From a naive reading it looks like this patch existing again is a mistake, since it's "fixed" in 1.11+. 😅
Is there an issue filed on https://github.com/golang/go we could link to for further discussion? Are they open to a knob for disabling this behavior in a future Go version that could re-deprecate this patch in the future?
There was a problem hiding this comment.
There is no issue for making this behavior configurable afaik
There was a problem hiding this comment.
No, AFAIK no one filed it. Makes sense to do so (at least to start a conversation). I'm pretty busy atm though :(
There was a problem hiding this comment.
@tonistiigi could you open a ticket? You're probably better at describing the problem than I am
thaJeztah
added
area/security
impact/changelog
kind/bugfix
|
@tonistiigi or @kolyshkin could one of you look at #40672 (comment) ? @tianon would you like to have the patch description updated as part of this PR (probably an extra commit, because this was cherry-picked from the release branch), or in a follow-up? |
|
Well, for context, I've already had two people see this patch description in the diff for the security update and get confused enough about whether it applies (given the description implies the problem the patch is fixing is already fixed in newer Go releases) that they reached out and asked me about it privately, so IMO the description really should be updated before this merges at the very least to make it clear it's an old patch being used to fix a new issue and is not in fact fixed by newer Go versions. 😅 |
|
Yes, this doesn't patch go but switches tar package from stdlib to our fork. Because vndr does not support stdlib packages natively some hackery is needed. An earlier version of this just used my fork branch like we do with some other vendors, but for better visibility of the changes, it was switched to using patch files. |
|
What's current status? |
|
needs rebase |
|
IMO the description in the patch file should either be updated or simply removed since it no longer applies to our usage, but otherwise I'm fine with this. 👍 |
This version avoids doing name lookups on creating tarball that should be avoided in to not hit loading glibc shared libraries. Signed-off-by: Tonis Tiigi <[email protected]> (cherry picked from commit aa6a989) Signed-off-by: Tibor Vass <[email protected]>
This blurb exists because we reused the same commit from an old patch, and thus got the commit message with it. However the message is confusing in this context. It was suggested in review that we should remove the confusing message. Signed-off-by: Brian Goff <[email protected]>
cpuguy83
force-pushed
the
19.03.8-forwardport-to-master
branch
from
702dfef to
841c1f3
Compare
|
This is rebased and the g1.11 message was removed (in a new commit). |
| @@ -11,10 +11,6 @@ on NULL pointer dereference). | |||
|
|
|||
| For much more details, see https://github.com/golang/go/issues/23265 | |||
There was a problem hiding this comment.
This link is also misleading, but at least it's not quite as confusing. 👍 ❤️
|
Ready to merge? |
7 participants
This version avoids doing name lookups on creating tarball that
should be avoided in to not hit loading glibc shared libraries.
Signed-off-by: Tonis Tiigi [email protected]
(cherry picked from commit aa6a989)
Signed-off-by: Tibor Vass [email protected]
Couple of changes from the commit that I cherry-picked: