Re: [PATCH] mapping of reactions

[Miloslav Trmac <mitr@redhat.com>]

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.