Re: [PATCH] mapping of reactions
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(a)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.