look(1): Capsicumise - #1489
Conversation
|
@oshogbo can you review this? |
|
@kfv Can you also fix the style issues reported by GitHub Actions? |
|
@oshogbo: The style checks are already passing, but I assume you're referring to the warnings for lines exceeding 80 characters. I’ll go ahead and address those as well, sure. |
8201453 to
cc71657
Compare
|
@oshogbo: I applied soft wrapping but kept |
cc71657 to
3a79009
Compare
|
Sorry, why haven't we used capsicum_helpers here? |
|
@markjdb any final comments? |
|
@kfv What are your plans here? This seems to be stuck waiting for addressing the helper feedback comments. |
|
Hi, apologies for the extended delay in addressing the requested changes. The past year has been personally and geopolitically challenging, and I appreciate your patience. I make sure to go through all the pending requests within the coming week at most. Thank you again for your understanding. |
|
I think this is ready for final review. Let me know if I’ve missed anything or if there’s anything else you'd like me to adjust. |
| err(EXIT_FAILURE, "failed to enter capability mode"); | ||
|
|
||
| for (size_t idx = 0; idx < nfiles; file = argv[idx++]) { | ||
| if (fstat(fds[idx], &sb)) |
There was a problem hiding this comment.
I think this is still wrong. I have not run this code, so I might be wrong here.
The open(2) returns -1 for failed open. We ignore actual error codem which is in errno. Then we pass -1 to fstat(2). In result instead of meaningful error like "File doesn't exists", "No access" ect. we provide user with a same error "Invalid file descriptor" when the fstat(2) fails.
There was a problem hiding this comment.
Right, that was my mistake. It’s now fixed, with the details explained here: #1489 (comment)
| if (caph_enter() != 0) | ||
| err(EXIT_FAILURE, "failed to enter capability mode"); | ||
|
|
||
| for (size_t idx = 0; idx < nfiles; file = argv[idx++]) { |
There was a problem hiding this comment.
Actually I just noticed that this magic is also wrong.
You forgot to reset file to argv[0]. So if I read this correctly your first file is actually the last from argv. Later it works.
This is why I actually don't like such magical initalization, I think such code like:
for (size_t idx = 0; idx < nfiles; idx++) {
file = argv[idx];
I haven't read into more details but it seems that we also move argv by one:
if (argc >= 2)
file = *argv++;
So I'm not sure if we can reuse it in the previous snipped. This all seems quite magical.
There was a problem hiding this comment.
You are absolutely right, Mariusz — that was my oversight, and thank you for catching it. My intention was to preserve the style of this decades-old codebase, which is why I avoided introducing structural modifications, and in doing so, I miscalculated the handling of argv. My apologies.
I have just pushed a new commit that addresses this by removing the implicit argv++ side effect and introducing a more explicit indexed loop over the files. And to keep track of errno, I added a small structure with fd and err members. I considered adding a name member as well, in which case the iteration domain would shift entirely to that structure, eliminating the need for argv in the second loop. It could be refactored even further if we decide it’s worthwhile.
If the preference is for a more substantial redesign, I am more than happy to work on that as well. Otherwise, please let me know if there are any remaining gaps for our strict "capsicumisation" goal here, or other areas that still need fixing, and I’ll take care of them.
122d423 to
8a3d887
Compare
| nfiles = argc > 1 ? argc - 1 : argc; | ||
| if ((files = malloc(nfiles * sizeof(struct files))) == NULL) | ||
| err(2, NULL); | ||
| for (size_t idx = 0; idx < nfiles; idx++) { |
There was a problem hiding this comment.
The code (at least for me) seem a little bit puzzling.
First I don't like the play with file. Its seems a little bit hackish.
Maybe we can have something like:
static char *_path_words[] = { _PATH_WORDS };
...
file_list = _path_words;
nfiles = 1;
if (argc >= 2) {
file_list = argv;
nfiles = argc - 1;
}
Then using file_list to iterate you don't have to guess which list you are iterating.
To be honest I don't like the fact that argv/argc gets desynchronized.
So I would propose also this change:
key = prepkey(*argv++, termchar);
argc -= 1;
There was a problem hiding this comment.
You're right. Can you take a look and see if the recent updates make it clean enough, or if you still think further improvements are needed? I've kept the ternaries as-is; I think with the rest of the modifications it should be clean enough, but I'm open to any ideas.
|
Is there a new version of this that addresses the review feedback? |
bsdimp
added
changes-required
|
Don't know why I forgot this PR, thought it's landed. Working on it now. |
|
Cc: @oshogbo |
|
LGTM. I will build it and test. |
|
May I proceed with this patch? Cc: @freebsdfrau, @clausecker |
|
Yes, please go ahead. |
|
Looking good here! |
Signed-off-by: Faraz Vahedi <kfv@kfv.io>
64a2d4d to
144f2ed
Compare
|
It's ready now. I'll land it shortly. |
Reviewed by: fuz, oshogbo Approved by: fuz (mentor) Pull Request: #1489
|
Closed by commit f66c868 |
6 participants
No description provided.