On Thursday, March 21, 2013 08:51:34 PM [email protected] wrote:
> From: Vitaly _Vi Shukela <[email protected]>
First off, a general comment, can I have your permission to add your sign-off
to these patches? Also, I'll go ahead and fix them up but in the future
submissions please be more mindful of whitespace, alignment, and the 80
character width limit.
More comments inline ...
> diff --git a/include/seccomp.h.in b/include/seccomp.h.in
> index b21205c..d3dce57 100644
> --- a/include/seccomp.h.in
> +++ b/include/seccomp.h.in
> @@ -389,6 +389,27 @@ int seccomp_syscall_priority(scmp_filter_ctx ctx,
> int seccomp_rule_add(scmp_filter_ctx ctx,
> uint32_t action, int syscall, unsigned int arg_cnt, ...);
>
> +
> +/**
> + * Add a new rule to the filter
> + * @param ctx the filter context
> + * @param action the filter action
> + * @param syscall the syscall number
> + * @param arg_cnt the number of argument filters in the argument filter
> chain
> + * @param scmp_arg_cmp array of scmp_arg_cmp structs (use of
> SCMP_ARG_CMP() recommended)
I understand this was probably just a cut-and-paste issue, but the
SCMP_ARG_CMP() macro isn't really applicable here. We also should use the
parameter name, arg_array, and not the type, scmp_arg_cmp, in the comment.
We should probably also tweak the comment for the arg_cnt parameter to
indicate it is the array size.
> diff --git a/src/api.c b/src/api.c
> index d70cc69..b8f30b3 100644
> --- a/src/api.c
> +++ b/src/api.c
> @@ -365,7 +365,8 @@ syscall_priority_failure:
> */
> static int _seccomp_rule_add(struct db_filter_col *col,
> unsigned int strict, uint32_t action, int syscall,
> - unsigned int arg_cnt, va_list arg_list)
> + unsigned int arg_cnt,
> + const struct scmp_arg_cmp *arg_array)
> {
> int rc = 0, rc_tmp;
> int sc_tmp;
> @@ -377,6 +378,9 @@ static int _seccomp_rule_add(struct db_filter_col *col,
> struct db_api_arg *chain = NULL, *chain_tmp;
> struct scmp_arg_cmp arg_data;
>
> + if (arg_array == NULL)
> + return -EINVAL;
I think we only want to return -EINVAL here if arg_array is NULL and arg_cnt
is greater than zero. We want to allow arg_array to be NULL is arg_cnt is
zero, e.g. a syscall only match. I probably should have been more clear when
we talked about this earlier.
> /* NOTE - function header comment in include/seccomp.h */
> +int seccomp_rule_add_array(scmp_filter_ctx ctx,
> + uint32_t action, int syscall, unsigned int arg_cnt,
> + const struct scmp_arg_cmp *arg_array)
> +{
We should probably validate arg_cnt like we do in the non-array functions.
> + return _seccomp_rule_add((struct db_filter_col *)ctx,
> + 0, action, syscall, arg_cnt, arg_array);
> +}
> +
> +
> +/* NOTE - function header comment in include/seccomp.h */
> int seccomp_rule_add(scmp_filter_ctx ctx,
> uint32_t action, int syscall, unsigned int arg_cnt, ...)
> {
> int rc;
> + int iter;
> + struct scmp_arg_cmp arg_array[ARG_COUNT_MAX];
> va_list arg_list;
>
> + if (arg_cnt < 0 || arg_cnt > ARG_COUNT_MAX)
> + return -1;
Don't return -1, use -EINVAL instead.
> va_start(arg_list, arg_cnt);
> - rc = _seccomp_rule_add((struct db_filter_col *)ctx,
> - 0, action, syscall, arg_cnt, arg_list);
> + for (iter = 0; iter < arg_cnt; ++iter)
> + arg_array[iter] = va_arg(arg_list, struct scmp_arg_cmp);
> +
> + rc = seccomp_rule_add_array(ctx, action, syscall, arg_cnt, arg_array);
> va_end(arg_list);
>
> return rc;
> }
>
> +
> +/* NOTE - function header comment in include/seccomp.h */
> +int seccomp_rule_add_exact_array(scmp_filter_ctx ctx,
> + uint32_t action, int syscall, unsigned int arg_cnt,
> + const struct scmp_arg_cmp *arg_array)
> +{
Same as above.
> + return _seccomp_rule_add((struct db_filter_col *)ctx,
> + 1, action, syscall, arg_cnt, arg_array);
> +}
> +
> +
> /* NOTE - function header comment in include/seccomp.h */
> int seccomp_rule_add_exact(scmp_filter_ctx ctx, uint32_t action,
> int syscall, unsigned int arg_cnt, ...)
> {
> int rc;
> + int iter;
> + struct scmp_arg_cmp arg_array[ARG_COUNT_MAX];
> va_list arg_list;
>
> + if (arg_cnt > ARG_COUNT_MAX || arg_cnt<0)
> + return -1;
Same.
> va_start(arg_list, arg_cnt);
> - rc = _seccomp_rule_add((struct db_filter_col *)ctx,
> - 1, action, syscall, arg_cnt, arg_list);
> + for (iter = 0; iter < arg_cnt; ++iter)
> + arg_array[iter] = va_arg(arg_list, struct scmp_arg_cmp);
> +
> + rc = seccomp_rule_add_exact_array(ctx, action, syscall, arg_cnt,
> arg_array); va_end(arg_list);
>
> return rc;
> }
>
> +
Pay attention to your diffs - clean this stuff up before submitting in the
future.
> /* NOTE - function header comment in include/seccomp.h */
> int seccomp_export_pfc(const scmp_filter_ctx ctx, int fd)
> {
--
paul moore
security and virtualization @ redhat
------------------------------------------------------------------------------
Own the Future-Intel® Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest.
Compete for recognition, cash, and the chance to get your game
on Steam. $5K grand prize plus 10 genre and skill prizes.
Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
_______________________________________________
libseccomp-discuss mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libseccomp-discuss
