Skip to content

Commit

Permalink
mxq_job: Fix invalid pointer deref on error path
Browse files Browse the repository at this point in the history
I've got this from valgrind:

    2020-04-15 16:11:00 +0200 mxqd[6617]: mxqd mxqd.c:2600:main(): DEBUG: group_cnt=36 :: 36 Groups loaded
    2020-04-15 16:11:00 +0200 mxqd[6617]: mxqd mxqd.c:1343:start_user(): DEBUG:  user=buczek(125) slots_to_start=11 :: trying to start jobs for user.
    2020-04-15 16:11:00 +0200 mxqd[6617]: mxqd mxqd.c:1370:start_user():   group=buczek(125):335148 slots_to_start=11 slots_per_job=11 :: trying to start job for group.
    ==6617== Conditional jump or move depends on uninitialised value(s)
    ==6617==    at 0x40318FB: free (vg_replace_malloc.c:540)
    ==6617==    by 0x415307: mxq_select_job_from_group (mxq_job.c:324)
    ==6617==    by 0x41545F: mxq_assign_job_from_group_to_daemon (mxq_job.c:355)
    ==6617==    by 0x416849: mxq_load_job_from_group_for_daemon (mxq_job.c:740)
    ==6617==    by 0x40715F: start_job (mxqd.c:1188)
    ==6617==    by 0x407B82: start_user (mxqd.c:1372)
    ==6617==    by 0x407CC1: start_user_with_least_running_global_slot_count (mxqd.c:1412)
    ==6617==    by 0x40B450: main (mxqd.c:2609)
    ==6617==
    2020-04-15 16:11:00 +0200 mxqd[6617]: mxqd mxq_job.c:746:mxq_load_job_from_group_for_daemon(): WARNING:   group_id=335148 :: mxq_assign_job_from_group_to_daemon(): No matching job found - maybe another server was a bit faster. ;)

The pointer freed at the end of `mxq_select_job_from_group()` is supposed
to be allocated by `mx_mysql_do_statement()`. However, the pointer is
only allocated in the common case, where the routine delivers a result:

    if (result && result->count && num_rows) {
        tmpdata = mx_calloc_forever(num_rows, size);
        ...
        *to = tmpdata;

In the unusual case that there was no result from the query (as above,
because other servers already started all pending jobs) or in error
cases, `job_id_out` might be uninitialized and the `free()` might
is invalid.

Make sure, we don't try to free via an unitialized pointer. Do not decide
based on the return value of `mx_mysql_do_statement()`, because we don't
want to duplicate the complicated logic in the if stated quited above to
be duplicated in the caller.
  • Loading branch information
donald committed Apr 15, 2020
1 parent ac79166 commit e2aa076
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions mxq_job.c
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ uint64_t mxq_select_job_from_group(struct mx_mysql *mysql, uint64_t group_id)
struct mx_mysql_bind param = {0};
struct mx_mysql_bind result = {0};
uint64_t job_id;
uint64_t *job_id_out;
uint64_t *job_id_out = NULL;
int res;

char *query =
Expand Down Expand Up @@ -321,7 +321,8 @@ uint64_t mxq_select_job_from_group(struct mx_mysql *mysql, uint64_t group_id)
} else {
job_id=0;
}
free(job_id_out);
if (job_id_out)
free(job_id_out);
return(job_id);
}

Expand Down

0 comments on commit e2aa076

Please sign in to comment.