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.
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.
LOL, looks like I had the above comment still pending? 😂
There was a problem hiding this comment.
Very sorry, I was not aware of this comment...
There was a problem hiding this comment.
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.
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.
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.
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.
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. |
kariya-mitsuru
force-pushed
the
check-chmod
branch
from
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]>
kariya-mitsuru
force-pushed
the
check-chmod
branch
from
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.