paste(1): capsicumise the utility #1443
Conversation
|
Cc: @oshogbo |
There was a problem hiding this comment.
This mostly looks ok to me, just some nitpicking.
I'm a bit wary about pre-opening all of the files at once. In general that's a bad idea since it might cause the utility to open more file descriptors than is allowed (small RLIMIT_NOFILE limits are not uncommon), but for paste that's probably ok...
I would also drop the sysexits change, as we generally don't add new uses of them.
usr.bin/paste/paste.c
Outdated
|
|
||
| cap_rights_init(&rights, CAP_READ); | ||
| STAILQ_FOREACH(lp, &lh, entries) { | ||
| if (lp->fp && cap_rights_limit(fileno(lp->fp), &rights) < 0 && |
There was a problem hiding this comment.
I would suggest calling cap_rights_limit() right after each fopen() call, rather than having a separate loop. It's easier to reason about that way (open+rights limiting happens in one go), and it's simpler.
Also, I don't think you need the lp->fp null check.
Also, I believe you can use caph_rights_limit() and avoid the ENOSYS check.
There was a problem hiding this comment.
I would suggest calling cap_rights_limit() right after each fopen() call, rather than having a separate loop. It's easier to reason about that way (open+rights limiting happens in one go), and it's simpler.
You're absolutely right, it's totally valid. Done.
Also, I don't think you need the lp->fp null check.
True.
Also, I believe you can use caph_rights_limit() and avoid the ENOSYS check.
True, but since it's concise enough for this small use case, I kept it as is. Let me know if you think it needs to be changed.
…d linked-list Signed-off-by: Faraz Vahedi <kfv@kfv.io>
Signed-off-by: Faraz Vahedi <kfv@kfv.io>
Signed-off-by: Faraz Vahedi <kfv@kfv.io>
Signed-off-by: Faraz Vahedi <kfv@kfv.io>
@markjdb: The termination status macros are from the standard C library (defined in |
Yes, apologies - I forgot about this distinction when reading several PRs back-to-back. Please ignore my comments relating to sysexits. |
Signed-off-by: Faraz Vahedi <kfv@kfv.io>
|
No worries, mate. Thanks! |
| rval = sequential(argv); | ||
| else | ||
| rval = parallel(argv); | ||
| for (filecnt = 0; *argv; ++argv, ++filecnt) { |
There was a problem hiding this comment.
Have you considered using fileargs?
man.freebsd.org/cgi/man.cgi?query=cap_fileargs&sektion=3&manpath=freebsd+14.1-release+and+ports
Opening files up front has some advantages, it's faster.
I wonder why you decided to open files upfront and not use fileargs?
There was a problem hiding this comment.
Lovely! I didn't know that, Mariusz. Thanks a lot, I'll take care of it.
| STAILQ_INSERT_TAIL(&lh, lp, entries); | ||
| } | ||
|
|
||
| if (cap_enter() < 0 && errno != ENOSYS) |
There was a problem hiding this comment.
I chose to use the raw Capsicum API instead of caph because it felt more concise and appropriate for the small-scale use cases in such a utility. Additionally, I thought having another direct example of capsicumisation with only raw API calls might be beneficial as a reference, considering further complexities observed here. While it does add some verbosity to the code, I don't see it causing any other significant issues. That said, if you still believe using Capsicum helpers would be better for this context, I’m happy to reconsider and follow your guidance.
| failed = errno; | ||
| } | ||
|
|
||
| if (cap_rights_limit(fileno(lp->fp), |
There was a problem hiding this comment.
Absolutely, but for the same reasons mentioned earlier, I felt it might be better to stick with the raw API. I’ve reviewed the helpers you’ve written, and they’re indeed excellent—adding no overhead or unnecessary abstraction. However, I thought it might still be worthwhile to use the raw API directly here. If you believe otherwise, I’m happy to adapt based on your recommendation.
There was a problem hiding this comment.
There is no need to use a raw API, capsicum helpers make it simpler.
Ideally when we make capsicum mandatory in FreeBSD it will also make code cleanup much simpler.
There was a problem hiding this comment.
Makes sense. OK, I can take care of it as well, rog.
|
|
||
| for (opencnt = cnt; opencnt;) { | ||
| for (output = 0, lp = head; lp; lp = lp->next) { | ||
| for (opencnt = filecnt; opencnt;) { |
There was a problem hiding this comment.
In opencnt can we do a logical expression?
There was a problem hiding this comment.
Do you mean using opencnt > 0? That’s certainly a valid approach and it’s a good idea if you want to make the code more self-explanatory. It avoids relying on implicit truthiness, which can sometimes be less clear. Personally, I prefer to keep the code brief and concise where possible, which is why I didn’t use that form. However, if you think it would be helpful for clarity, I’d be open to considering the change.
There was a problem hiding this comment.
Yes, I prefer to have a boolean expression in conditions it makes it easier to understand.
There was a problem hiding this comment.
Roger that, I'll take care of it.
|
Thank you for taking the time to contribute to FreeBSD!
|
This PR applies Capsicum security enhancements to the
pasteutility as part of my initial contribution with Capsicumisation. The user experience and POSIX compliance have been preserved, with adjustments made to improve the code’s structure and error handling.My goal is to gain a deeper understanding of Capsicum by starting with simpler utilities before progressing to more complex codebases and libraries, so any corrections, suggestions, or feedback on the approach are highly appreciated, as I am eager to refine my work and contribute more broadly to the project.