Skip to content

mxq_job: Fix invalid pointer deref on error path #84

Merged
merged 1 commit into from
Apr 15, 2020
Merged

Conversation

donald
Copy link
Contributor

@donald donald commented Apr 15, 2020

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.

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.
@donald donald merged commit 5d1f3da into master Apr 15, 2020
@donald donald deleted the fix-invalid-free branch October 28, 2022 14:22
Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant