-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Check the validity of the chmod option arguments for COPY and ADD #5148
Conversation
@Chenbinx Do you remember why this used @kariya-mitsuru Ideally, we would have a test for this as well. |
Sorry, I haven't maintained related projects for a long time due to work transfer. I just looked at the code and really couldn't remember the reason. I'm sorry I didn't help.
在 2024年7月13日,02:09,Tõnis Tiigi ***@***.***> 写道:
@Chenbinx<https://github.com/Chenbinx> Do you remember why this used err == nil #1492<#1492> ? Stricter validation seems to make more sense to me.
@kariya-mitsuru<https://github.com/kariya-mitsuru> Ideally, we would have a test for this as well.
—
Reply to this email directly, view it on GitHub<#5148 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAJS75WNDYE7UUK2DZAIRPDZMALVTAVCNFSM6AAAAABKVNNQYSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRWGEZDCOJRHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Sorry for the slow response. |
perm := os.FileMode(p) | ||
mode = &perm | ||
if err != nil { | ||
return errors.New("invalid chown format") |
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.
Looks like the error message refers to "chown", not "chmod". Also curious if the original error should be preserved (although maybe it's not very informative)
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.
LOL, looks like I had the above comment still pending? 😂
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.
Very sorry, I was not aware of this comment...
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.
no! no need to apologise; I thought I submitted that one already, but looks like my review was still pending (but I saw you already found the typo 😄 🎉)
perm := os.FileMode(p) | ||
mode = &perm | ||
if err != nil { | ||
return fmt.Errorf("invalid chmod format: '%v'", cfg.chmod) |
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.
Can you use errors.Errorf
for this one? I think using pkg/errors
is preferred in this project (as those errors contain additional data that's used for tracing)
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.
Thank you.
Based on the above comment, I also tried errors.Wrap
, but the error message was as below, which didn't seem very useful for me, so I tried using errors.Errorf
.
ERROR: failed to solve: invalid chmod format: strconv.ParseUint: parsing "64a": invalid syntax
If the above message is more appropriate, please feel free to let me know.
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.
Yeah, I don't think the ParseUint
error is very useful. Perhaps the error message could mention that it should be a valid octal value, but otherwise it's probably fine as-is.
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.
the error message could mention that it should be a valid octal
Oh, certainly!
Furthermore, I have found that if the value is 0o3777777777777
(-1), it is ignored, and otherwise all but the lower 12 bits are meaningless.
I will try to change it again and add more test later.
Sorry, the error message changed did not pass golangci-lint. |
@kariya-mitsuru When making small updates to the unmerged commits in the PR, try to squash them together with existing commits for cleaner commit history. |
b88f7e0
to
4d7fd61
Compare
The current implementation silently ignores the chmod option argument for COPY and ADD if it is invalid, so this PR change to check the validity of the argument. I also found that if the value is 0o3777777777777(-1), it is ignored, and otherwise all but the lower 12 bits are meaningless. Signed-off-by: Mitsuru Kariya <[email protected]>
4d7fd61
to
6fddb5d
Compare
The current implementation silently ignores the chmod option argument for COPY and ADD if it is invalid, so this PR change to check the validity of the argument.