From e2aa076149bd3acb07f689889d7d2f0f3a3d08d0 Mon Sep 17 00:00:00 2001
From: Donald Buczek <buczek@molgen.mpg.de>
Date: Wed, 15 Apr 2020 16:22:09 +0200
Subject: [PATCH] mxq_job: Fix invalid pointer deref on error path

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.
---
 mxq_job.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mxq_job.c b/mxq_job.c
index ee3b3918..5a0041a5 100644
--- a/mxq_job.c
+++ b/mxq_job.c
@@ -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 =
@@ -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);
 }