Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap-confine: various small fixes and tweaks to seccomp support code #3518
Conversation
jdstrand
reviewed
Jun 23, 2017
These are nice changes and thanks for spotting the lack of NULL checks (shame that wasn't found in the initial PR).
I didn't have an opportunity to review the wait PR; can you use strtol() instead of atoi()? While the use of atoi() looks ok here, strtol() is best practice.
| - die("Out of memory creating checked_path"); | ||
| + size_t checked_path_size = strlen(path) + 1; | ||
| + char *checked_path __attribute__ ((cleanup(sc_cleanup_string))) = NULL; | ||
| + checked_path = calloc(checked_path_size, 1); |
jdstrand
Jun 23, 2017
Contributor
I know that '1' is equivalent to sizeof(char), but I think that sizeof(char) declares intent of use better for the calloc and to a lesser extent checked_path_size.
| + if (MAX_PROFILE_WAIT != NULL) { | ||
| + int env_max_wait = atoi(MAX_PROFILE_WAIT); | ||
| + max_wait = env_max_wait > 0 ? env_max_wait : max_wait; | ||
| + } |
jdstrand
Jun 23, 2017
Contributor
Can we also clean this up to use strtol() instead of atoi() (it has better error checking). Eg:
long max_wait = 120;
const char *MAX_PROFILE_WAIT = getenv("SNAP_CONFINE_MAX_PROFILE_WAIT");
if (MAX_PROFILE_WAIT != NULL) {
char *endptr = NULL;
errno = 0;
long env_max_wait = strtol(MAX_PROFILE_WAIT, &endptr, 10);
if (errno != 0 || MAX_PROFILE_WAIT == endptr || *endptr != '\0' || env_max_wait <= 0) {
die("SNAP_CONFINE_MAX_PROFILE_WAIT invalid");
}
max_wait = env_max_wait > 0 ? env_max_wait : max_wait;
}
...
| - perror | ||
| - ("prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, ...) failed"); | ||
| - die("aborting"); | ||
| + if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog) < 0) { |
jdstrand
Jun 23, 2017
Contributor
I prefer the specificity of != 0 rather than < 0 here and above, but these changes are not wrong.
|
I'll close and address feedback shortly. |
zyga
closed this
Jul 5, 2017
zyga
added some commits
Jun 23, 2017
Some checks were not successful
zyga
reopened this
Jul 5, 2017
codecov-io
commented
Jul 5, 2017
•
Edited 1 time
-
codecov-io
Jul 6, 2017
-
Jul 6, 2017
Codecov Report
@@ Coverage Diff @@
## master #3518 +/- ##
==========================================
- Coverage 76.76% 76.75% -0.01%
==========================================
Files 379 379
Lines 26277 26277
==========================================
- Hits 20171 20169 -2
- Misses 4314 4315 +1
- Partials 1792 1793 +1
Continue to review full report at Codecov.
|
| + || env_max_wait <= 0) { | ||
| + die("SNAP_CONFINE_MAX_PROFILE_WAIT invalid"); | ||
| + } | ||
| + max_wait = env_max_wait > 0 ? env_max_wait : max_wait; |
jdstrand
Jul 5, 2017
Contributor
You might overflow max_wait here. You should use long max_wait = 120 up above to avoid that (or cap the maximum wait to something 'reasonable' (eg, 3600) that fits in max_wait). This will only effectively skip the for loop, but should fix it regardless.
zyga commentedJun 23, 2017
This branch is a result of top-to-bottom read and tweak of seccomp support code.
There are several fixes for missing NULL checks, simplifications to memory management,
consistency tweaks for error messages, for error checks and for const correctness.