Re: [PATCH] mapping of reactions

Miloslav Trmac
2010-03-31T18:36:29Z

Hello,
the code looks reasonable, some minor comments are below. I'll let Steve and others comment on the high-level design (just to point out a question, is it OK that auditctl will depend on sqlite?).
Mirek

----- "Juraj Hlista" <juro.hlista@gmail.com> wrote:
> diff --git a/lib/libaudit.c b/lib/libaudit.c
> @@ -965,6 +983,14 @@ int audit_rule_fieldpair_data(struct
> audit_rule_data **rulep, const char *pair,
> strncpy(&rule->buf[offset], v, vlen);
>
> break;
> + case AUDIT_REACTION:
> + /* string identifiers were converted to numbers */
> + if (isdigit((char)*(v)))
Nitpick: the isdigit argument should be cast to (unsigned char).

> diff --git a/lib/reactarray.c b/lib/reactarray.c
<snip>
> +int react_array_init(struct react_array *a, unsigned int size)
<snip>
> + a->str = (char **)malloc(size * sizeof(char *));
The return value of malloc() is not usually manually cast in C.
> + if (!a->str)
> + return 1;
> +
> + for (i = 0; i < size; i++)
> + a->str[i] = NULL;
You can just use calloc() to initialize a->str.
<snip>
> +void react_array_free(struct react_array *a)
<snip>
> + for (i = 0; i < a->count; i++) {
> + if (a->str[i])
> + free(a->str[i]);
free(NULL) is OK, so the if ( ) is not necessary.

> +int react_array_insert(struct react_array *a, const char *s)
> +{
<snip>
> + a->str[a->count] = (char *)malloc((strlen(s) + 1) * sizeof(char));
> + if (!a->str[a->count])
> + return 1;
> +
> + strcpy(a->str[a->count], s);
Using strdup() would be simpler.

> diff --git a/src/auditctl-reactsql.c b/src/auditctl-reactsql.c
<snip>
> +enum {
> + SQL_CHECK_DB = 0,
Just use string constants in the code directly, this indirection is difficult to follow.
<snip>
> +void sql_print_error(sqlite3 *c, int err)
<snip>
> + fprintf(stderr, "SQLite error: %s\n", sql_errmsg[-err - 2]);
The "-2" is a bit difficult to follow... I'd just sacrifice the two additional empty entries in sql_errmsg.
<snip>
> +int sql_number_to_reaction(sqlite3 *c, const int num, char **str)
<snip>
> + *str = malloc((strlen(reaction) + 1) * sizeof(char));
> + if (*str == NULL) {
> + sqlite3_finalize(find_str);
> + return -SQL_NO_MEMORY;
> + }
> + strcpy(*str, reaction);
Use strdup ().

<snip>
> +/*
> + * Add a reaction to the database - if 'num' is greater than SQL_OFFSET,
> + * a reaction identifier (string) is already in the database and only
> + * 'used' is incremented. If there is not such a reaction string, a new
> + * one is inserted into the database and 'used' is set to 1.
> + */
Using a separate variable for "new/existing" would be much cleaner than the magic SQL_OFFSET. Especially see how this implementation detail leaks into auditctl.c.
<snip>
> +int sql_get_next_number(sqlite3 *c, const char *str)
Here as well.

> diff --git a/src/auditctl.c b/src/auditctl.c
> @@ -917,6 +972,97 @@ static int setopt(int count, int lineno, char
<snip>
> + if (num > SQL_OFFSET)
> + asprintf(&cmd, "react=%u", num - SQL_OFFSET);
> + else
> + asprintf(&cmd, "react=%u", num);
> + if (cmd) {
(...)
> + } else {
> + fprintf(stderr,
> + "Out of memory adding reaction\n");
> + sql_close_database(conn);
> + return -4;
> + }
If you reverse the if (cmd) here, the else {} branch becomes the default control flow, resulting in a bit simpler code.

> @@ -1022,6 +1168,7 @@ static int fileopt(const char *file)
>
> /* Parse it */
> if (reset_vars()) {
> + free_vars();
I didn't look in detail, this does not match my understanding of "reset_vars()"; reset_vars() is supposed to reinitialize everything for a next command, not free everything. (The free(rule_new) call you moved from reset_vars() to free_vars() was at the beginning of reset_vars(), not at the end.)

> @@ -1382,6 +1569,25 @@ static int audit_print_reply(struct audit_reply
<snip>
> + rc = sql_number_to_reaction(conn,
> + rep->ruledata->values[i],
> + &str_react);
> + if (rc < 0) {
I think it's prefereble to print the number if the lookup fails, so that the admin can see at least something from the rule.