-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
attach: fix attach when cuid is too long #1704
attach: fix attach when cuid is too long #1704
Conversation
LGTM |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude 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 |
f90c6be
to
8144a3f
Compare
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.
LGTM assuming happy tests.
Should this patch be in a separate Unix/Linux only code path? |
8144a3f
to
2c55a41
Compare
not sure, do we support anything else than Unix? |
also I am still unsure how to fix conmon (and then again in podman) when the substring gives a path that is already existing. It is a corner case but still can happen |
12d4b63
to
4aab5ee
Compare
and patch for conmon to fix the symlink length corner case: cri-o/cri-o#1875 |
I am really concerned about what happens during collisions - if we start a few hundred to a thousand containers, do we start hitting this frequently? We should try and make sure that a sizable portion of the UUID makes it in there. If we manually specify a libpod run path long enough, we could end up generating completely unusable attach paths like this (cut off everything except part of the path to the attach sockets directory) |
I think we should just error out and not accept any runroot that is longer than 40-50 characters so that we have enough room for the UUID and avoid such kind of issues. What do you think? I can add another patch to this PR if you like the idea. |
4aab5ee
to
3cd27a6
Compare
That sounds good too me - as long as we get most of the container ID in
there, we shouldn't have to worry about collisions
…On Fri, Oct 26, 2018, 03:27 Giuseppe Scrivano ***@***.***> wrote:
I am really concerned about what happens during collisions - if we start a
few hundred to a thousand containers, do we start hitting this frequently?
I think we should just error out and not accept any runroot that is longer
than 40-50 characters so that we have enough room for the UUID and avoid
such kind of issues. What do you think? I can add another patch to this PR
if you like the idea.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1704 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHYHCMDl6tpELgpnsz-4ZSsozXKa8erYks5uorlygaJpZM4X1G0L>
.
|
3cd27a6
to
a471e85
Compare
/retest |
a471e85
to
597ebfd
Compare
socketPath = socketPath[0:maxUnixLength] | ||
} | ||
|
||
logrus.Debug("connecting to socket ", socketPath) |
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.
I think that should be Debugf("... %v", socketPath)
.
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.
+1, I'm surprised gofmt or govet didn't ping on this.
tests are passing and I've added the additional check |
LGTM but I prefer to wait for @mheon's final ack. |
|
||
maxUnixLength := int(C.unix_path_length()) | ||
if maxUnixLength < len(socketPath)-1 { | ||
socketPath = socketPath[0:maxUnixLength] |
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.
Do we need to subtract one from maxUnixLength here to account for a trailing null?
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.
thanks for catching it. No it is not needed, I've tried both versions and I am surprised it doesn't crash. I've dropped the -1
and pushed a new version
One question on a possible off-by-one. Could just be me seeing things that aren't there, coffee isn't working yet |
conmon creates a symlink to avoid using a too long UNIX path. Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1641800 There is still one issue when the path length of the symlink has the same length of the attach socket parent directory since conmon fails to create the symlink, but that must be addressed in conmon first. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
597ebfd
to
c65b359
Compare
LGTM, @mheon you're it |
/lgtm |
conmon creates a symlink to avoid using a too long UNIX path.
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1641800
There is still one issue when the path length of the symlink has the
same length of the attach socket parent directory since conmon fails
to create the symlink, but that must be addressed in conmon first.
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com