From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miloslav Trmac Subject: Re: [PATCH] mapping of reactions Date: Wed, 31 Mar 2010 14:36:29 -0400 (EDT) Message-ID: <2098439088.299041270060589834.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com> References: <1888242976.297641270059855277.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1888242976.297641270059855277.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Juraj Hlista Cc: linux-audit@redhat.com List-Id: linux-audit@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" 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 > +int react_array_init(struct react_array *a, unsigned int size) > + 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. > +void react_array_free(struct react_array *a) > + 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) > +{ > + 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 > +enum { > + SQL_CHECK_DB = 0, Just use string constants in the code directly, this indirection is difficult to follow. > +void sql_print_error(sqlite3 *c, int err) > + 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. > +int sql_number_to_reaction(sqlite3 *c, const int num, char **str) > + *str = malloc((strlen(reaction) + 1) * sizeof(char)); > + if (*str == NULL) { > + sqlite3_finalize(find_str); > + return -SQL_NO_MEMORY; > + } > + strcpy(*str, reaction); Use strdup (). > +/* > + * 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. > +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 > + 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 > + 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.