look(1): Capsicumise #1489
look(1): Capsicumise #1489
Conversation
|
@oshogbo can you review this? |
| do { | ||
| if ((fd = open(file, O_RDONLY, 0)) < 0 || fstat(fd, &sb)) | ||
| #ifndef WITHOUT_CAPSICUM | ||
| cap_rights_init(&rights, CAP_MMAP_R, CAP_READ, CAP_FSTAT); |
There was a problem hiding this comment.
Why we need WITHOUT_CAPSICUM, look doesn't seem to be in contrib.
Seems like using capsicum_helpers also might simplify the process.
There was a problem hiding this comment.
I used WITHOUT_CAPSICUM assuming it is required to handle cases where the kernel is built without Capsicum support, rather than strictly for tools in contrib. Could you clarify if my understanding is off? I'd appreciate any insights!
There was a problem hiding this comment.
On FreeBSD if you build a kernel without the CAPSICUM support the cap_* functions still exists however they will fail and return ENOSYS.
To avoid checking errno you can use capsicum helpers.
The WITHOUT_CAPSICUM is for platforms that aren't aware of capsicum's existence, like Linux. So we have to add them to contrib source code which isn't managed by FreeBSD but not in the code that is dedicated for FreeBSD.
There was a problem hiding this comment.
Thanks, I didn't know that. I'll take care of it.
| #ifndef WITHOUT_CAPSICUM | ||
| cap_rights_init(&rights, CAP_MMAP_R, CAP_READ, CAP_FSTAT); | ||
| #endif | ||
| int fds[argc > 1 ? argc - 1 : argc]; |
There was a problem hiding this comment.
We don't use dynamic stack variable initialization from newer C. You should use malloc or calloc.
There was a problem hiding this comment.
I didn't find anything in style(9) explicitly disallowing VLAs, and technically they’re not from 'newer' C—VLAs have been part of the standard since C99 and are mandatory again in C23 (after being optional since C11.) I believe using a VLA here would simplify our approach significantly and avoid unnecessary heap allocation, improving memory efficiency. I’d like to request reconsideration for this specific case, but if there’s a strong rationale against VLAs in general, I'd appreciate any guidance on it.
There was a problem hiding this comment.
Yes, you are right. However, I (maybe I should say We as project) try to avoid them. Stack is limited and I don't want to even analyze if we won't exceed it on all supported platforms or if the command line lenght limit will be enough.
There was a problem hiding this comment.
All right, it makes sense.
|
@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. |
kfv
force-pushed
the
kfv/capsicum/look
branch
from
8201453
to
cc71657
Compare
|
@oshogbo: I applied soft wrapping but kept |
Signed-off-by: Faraz Vahedi <[email protected]>
kfv
force-pushed
the
kfv/capsicum/look
branch
from
cc71657
to
3a79009
Compare
No description provided.