Skip to content
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

utils: drop check for invalid path #843

Merged
merged 1 commit into from
Jan 11, 2022

Conversation

giuseppe
Copy link
Member

it has already caused an issue in the past and it doesn't add any
value, since each component in the path is validated again later on.

Closes: #842

Signed-off-by: Giuseppe Scrivano [email protected]

@rhatdan
Copy link
Member

rhatdan commented Jan 10, 2022

LGTM
@flouthoc PTAL

@flouthoc
Copy link
Collaborator

PR looks good. But its still in draft and fuzzing shows hiccups.

@kolyshkin
Copy link
Collaborator

and fuzzing shows hiccups.

I guess I'm the one to blame for this.

What we see in CI is exit code 141 which means SIGPIPE; most probably this is because of ff3e33b. Since pipefail option is used, and grep exits once the match is found, the left side of the pipe (shell running echo built-in) could get a sigpipe. I think it was prevented by having a subshell, which I removed.

Anyway, here's a fix: #844

it has already caused an issue in the past and it doesn't add any
value, since each component in the path is validated again later on.

Closes: containers#842

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the drop-invalid-path-check branch from 71e7af7 to 4cd65c3 Compare January 11, 2022 08:50
@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2022

Should this PR be closed or merged?

@giuseppe giuseppe marked this pull request as ready for review January 11, 2022 12:05
@giuseppe
Copy link
Member Author

ready to merge

@flouthoc
Copy link
Collaborator

Looks good to me and change is verified hence merging this.

@flouthoc flouthoc merged commit 9f6d8e0 into containers:main Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New flake: invalid path etc/hosts: OCI runtime error
4 participants