Skip to content
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.

Sign up for GitHub

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

look(1): Capsicumise #1489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+25 −7
Open

look(1): Capsicumise #1489

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions usr.bin/look/look.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
*/

#include <sys/types.h>
#include <sys/capsicum.h>
#include <sys/mman.h>
#include <sys/stat.h>

Expand Down Expand Up @@ -89,11 +90,12 @@
main(int argc, char *argv[])
{
struct stat sb;
int ch, fd, match;
int ch, match;
wchar_t termchar;
unsigned char *back, *front;
unsigned const char *file;
wchar_t *key;
cap_rights_t rights;

(void) setlocale(LC_CTYPE, "");

Expand Down Expand Up @@ -132,21 +134,37 @@

match = 1;

do {
if ((fd = open(file, O_RDONLY, 0)) < 0 || fstat(fd, &sb))
cap_rights_init(&rights, CAP_MMAP_R, CAP_READ, CAP_FSTAT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need WITHOUT_CAPSICUM, look doesn't seem to be in contrib.
Seems like using capsicum_helpers also might simplify the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I didn't know that. I'll take care of it.

int fds[argc > 1 ? argc - 1 : argc];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use dynamic stack variable initialization from newer C. You should use malloc or calloc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, it makes sense.

for (size_t idx = 0;
idx < sizeof(fds) / sizeof(fds[0]);
file = argv[idx++]) {
if ((fds[idx] = open(file, O_RDONLY, 0)) < 0)
err(2, "%s", file);
if (cap_rights_limit(fds[idx], &rights) < 0 && errno != ENOSYS)
err(2, "unable to limit rights for %s", file);
}

if (cap_enter() < 0 && errno != ENOSYS)
err(EXIT_FAILURE, "failed to enter capability mode");

for (size_t idx = 0;
idx < sizeof(fds) / sizeof(fds[0]);
file = argv[idx++]) {
if (fstat(fds[idx], &sb))
err(2, "%s", file);
if ((uintmax_t)sb.st_size > (uintmax_t)SIZE_T_MAX)
errx(2, "%s: %s", file, strerror(EFBIG));
if (sb.st_size == 0) {
close(fd);
close(fds[idx]);
continue;
}
if ((front = mmap(NULL, (size_t)sb.st_size, PROT_READ, MAP_SHARED, fd, (off_t)0)) == MAP_FAILED)
if ((front = mmap(NULL, (size_t)sb.st_size, PROT_READ, MAP_SHARED, fds[idx], (off_t)0)) == MAP_FAILED)

Check warning on line 162 in usr.bin/look/look.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
err(2, "%s", file);
back = front + sb.st_size;
match *= (look(key, front, back));
close(fd);
} while (argc-- > 2 && (file = *argv++));
close(fds[idx]);
}

exit(match);
}
Expand Down
Loading