I just got a crash while trying to make a delivery to a non-existant user. I
skipped past the part that made sure that the user exists and so was able to
discover a pretty significant bad idiom that's widely used in db.c
The problem is that db_get_result() can return NULL if there's actually no
results to find. In a number of cases, the count of returned rows in not
checked, db_get_result() is called, and it is not tested for a valid return
before being derefenced or passed as an argument to a standard library
function, such as strtoull(), that apparently cannot handle a NULL first
argument.
In a few places there is a better idiom (this is from db_createsession():
tmpmessage.msize =
query_result ? strtoull(query_result, NULL, 10) : 0;
db.c should really be audited and this better idiom put wherever possible.
Note that it is entirely possible to have a situation in which even after
checking everything that indicates that a value should be returned,
db_get_result() can still return a NULL value; it's output should always be
checked!
Alternatively, since at least half of the calls to db_get_result() are fed
into strtoull() right away, why not have a function that does the proper
result validation and then makes the call to strtoull() and returns a u64_t?
Aaron
