So 11 cases involve direct-to-strtoull(), which means crashing if NULL, 24
cases have some variation of query ? strtoull() : 0 and these two are the
questionable cases. The first one looks like a problem, the second one isn't a
problem if the flags[] array's values are either 0 or 1.
I'll write up the db_get_result_u64() function sometime today along with the
changes to db.c and post it to SourceForge and/or the mailing list.
This is the only chunk that I found that handles NULL separately from 0:
result_string = db_get_result(0, 0);
if (result_string)
*message_size = strtoull(result_string, NULL, 10);
else {
trace(TRACE_ERROR,
"%s,%s: no result set after requesting msgsize "
"of msg [%llu]\n",
__FILE__, __FUNCTION__, message_idnr);
db_free_result();
return -1;
}
db_free_result();
return 1;
This one looks like it treats NULL separately, but I'd bet that 0 is the other
possible value for the flags field:
if (db_num_rows() > 0) {
for (i = 0; i < IMAP_NFLAGS; i++) {
query_result = db_get_result(0, i);
if (query_result && query_result[0] != '0')
flags[i] = 1;
}
}
Here's a funny one, btw, which looks like a bizarre glorified strdup() to me:
if (!(tmp_name = my_malloc((tmp_name_len + 1) * sizeof(char)))) {
trace(TRACE_ERROR,"%s,%s: error allocating memory",
__FILE__, __FUNCTION__);
return -1;
}
strncpy(tmp_name, query_result, tmp_name_len);
tmp_name[tmp_name_len] = '\0';
Aaron
Ilja Booij <ilja@ic-s.nl> said:
> Hi
>
> Aaron Stone wrote:
>
> > Since directly testing the return value is easy enough, we should do that
> > rather than test other things that *should* mean that the return value is
> > non-NULL. Based on some grepping, literally 55% of calls to db_get_result()
> > are passed to strtoull() within the next two lines of code. 11 calls are on
> > the same line of code (presumably waiting to die when db_get_result() returns
> > a NULL for whatever reasons it might -- no data, db error, etc.)
> You're right. Otherwise a new error is introduced way to easily. :)
> >
> > [dbmail-2.0rc2]$ grep db_get_result db.c | wc
> > 67 298 2930
> > [dbmail-2.0rc2]$ grep db_get_result db.c | grep strtoull | wc
> > 11 64 645
> > [dbmail-2.0rc2]$ grep db_get_result -A1 db.c | grep strtoull | wc
> > 19 132 1194
> > [dbmail-2.0rc2]$ grep db_get_result -A2 db.c | grep strtoull | wc
> > 37 234 2143
> > [dbmail-2.0rc2]$ grep db_get_result -A10 db.c | grep strtoull | wc
> > 37 234 2143
> >
> >
> > Since it's only 37 cases, I'll go through and look to see if any of them have
> > special handling for the NULL case, or if they just use 0. If it turns out
> > that way, then I'll write up a db_get_result_u64() function.
> Great, sounds like a plan. :D
>
> Ilja
>
> > Ilja Booij <ilja@ic-s.nl> said:
> >
> >
> >>Hi,
> >>
> >>I wouldn't now actually.. I guess there are functions for which strtoull
> >>can return a 0.
> >>
> >>auth_getmaxmailsize() is an example of this. When the maxmail_size for a
> >>user is 0, strtoull will return 0.
> >>
> >>I agree with the fact that we need to clean up the code in db.c to deal
> >>with db_get_result() returning NULL.
> >>
> >>Maybe we should pay more attention to the reason why db_get_result()
> >>returns 0.
> >>
> >>In dbpgsql.c/dbmysql.c, db_get_result() will return 0 if the result set
> >>is NULL, of if the row, field combination asked is not in the result
> >>set. Before every db_get_result() we could make sure we are only asking
> >>for a valid row, by making sure the row_nr we are asking for is <
> >>db_num_rows().
> >>db_num_rows() will return 0 if there are no rows in the reslt set. If
> >>this is the case, no call to db_get_result() should be done.
> >>
> >>Still, I guess we should make sure that no call will be made to
> >>strtoull() when the query_result is NULL.
> >>
> >>Ilja
> >>
> >>Aaron Stone wrote:
> >>
> >>
> >>>It occurs to me that the correct solution really is to have a function that
> >>>integrates a properly validated strtoull() call, but what I can't figure out
> >>>is how to determine the difference between a 0 and a NULL result -- that is,
> >>>if we get a NULL within the function, and return 0, it's no different than
> >>>actually getting 0. Are there potential consumer functions that need to
> >>>differentiate these situations?
> >>>
> >>>Aaron
> >>>
> >>>
> >>>"Aaron Stone" <aaron@serendipity.palo-alto.ca.us> said:
> >>>
> >>>
> >>>
> >>>>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
> >>>>_______________________________________________
> >>>>Dbmail-dev mailing list
> >>>>Dbmail-dev@dbmail.org
> >>>>http://twister.fastxs.net/mailman/listinfo/dbmail-dev
> >>>>
> >>>
> >>>
> >>>
> >>>
> >>_______________________________________________
> >>Dbmail-dev mailing list
> >>Dbmail-dev@dbmail.org
> >>http://twister.fastxs.net/mailman/listinfo/dbmail-dev
> >>
> >
> >
> >
> >
> _______________________________________________
> Dbmail-dev mailing list
> Dbmail-dev@dbmail.org
> http://twister.fastxs.net/mailman/listinfo/dbmail-dev
>
--