On Wed, May 11, 2011 at 5:09 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Wed, May 11, 2011 at 04:17:12PM -0700, elij wrote:
make the sql query form consistent in usage by cleaning up instances where db_query's result was not inspected before attempting to fetch row data from the handle --- web/html/addvote.php | 16 +++++++++- web/html/tu.php | 17 +++++++++-- web/lib/acctfuncs.inc | 59 ++++++++++++++++++++++++---------------- web/lib/aur.inc | 8 ++++- web/lib/pkgfuncs.inc | 43 +++++++++++++++++++++--------- web/template/actions_form.php | 52 ++++++++++++++++++++---------------- web/template/tu_list.php | 8 +++++- 7 files changed, 135 insertions(+), 68 deletions(-)
Looks quite ok now. Which method did you use to spot these inconsistencies?
After I found and noticed one (was working on some code that triggered a php warning due to the issue, I did the following grep to find more instances. I was primarily looking for instances where db_query was being _directly_ passed to mysql_fetch_* functions. grep -R db_query /path/to/web/ | grep mysql
Skimming through the source code it seems that there are some more query results that should be validated, like the "SELECT" one in "web/html/pkgsubmit.php".
Yeah, I didn't originally look for these, but I found some more with this.. grep -R -n -A1 db_query * | grep -B1 mysql That find instances where db_query is directly followed (next line) by a mysql_ function. Looking at these a few are cases of 'not testing return value of db_query before eval'. output of above command: # looks like it needs to be fixed html/pkgsubmit.php:303: $result = db_query($q, $dbh); html/pkgsubmit.php-304- $pdata = mysql_fetch_assoc($result); # this one look relatively ok. just an insert. html/pkgsubmit.php:352: db_query($q, $dbh); html/pkgsubmit.php-353- $packageID = mysql_insert_id($dbh); # looks like it needs to be fixed html/account.php:51: $result = db_query($q, $dbh); html/account.php-52- if (!mysql_num_rows($result)) { # looks like it needs to be fixed html/account.php:78: $result = db_query($q, $dbh); html/account.php-79- if (!mysql_num_rows($result)) { # looks like it needs to be fixed html/account.php:115: $result = db_query($q, $dbh); html/account.php-116- if (!mysql_num_rows($result)) { # looks like it needs to be fixed html/tu.php:28: $results = db_query($q, $dbh); html/tu.php-29- $row = mysql_fetch_assoc($results); # looks like it needs to be fixed html/tu.php:53: $result = db_query($qwhoVoted,$dbh); html/tu.php-54- if (mysql_num_rows($result) > 0) { # this one is probably ok. it uses the handle to get a 'changed row count' html/passreset.php:45: $result = db_query($q, $dbh); html/passreset.php-46- if (!mysql_affected_rows($dbh)) { # looks like it needs to be fixed lib/pkgfuncs.inc:254: $result = db_query($q, $dbh); lib/pkgfuncs.inc-255- if (mysql_num_rows($result) > 0) { # looks like it needs to be fixed lib/pkgfuncs.inc:637: $result = db_query($q, $dbh); lib/pkgfuncs.inc-638- if (mysql_num_rows($result)) { # looks like it needs to be fixed lib/pkgfuncs.inc:879: $result = db_query($q, $dbh); lib/pkgfuncs.inc-880- if (!mysql_num_rows($result)) { # looks like it needs to be fixed lib/stats.inc:24: $result = db_query($dbq, $dbh); lib/stats.inc-25- $row = mysql_fetch_row($result); # looks like it needs to be fixed lib/aur.inc:30: $result = db_query($q, $dbh); lib/aur.inc-31- if (mysql_num_rows($result) == 0) { # looks like it needs to be fixed lib/aur.inc:348: $result = db_query($q, $dbh); lib/aur.inc-349- if (mysql_num_rows($result) == 0) {return 1;} # looks like it needs to be fixed template/pkg_comment_form.php:31: $result = db_query($q, $dbh); template/pkg_comment_form.php-32- $row = mysql_fetch_assoc($result); Some of this could probably be cleaned up through judicious use of a few wrapper functions, with the added benefit of abstracting the sql away like we had previously talked about... Still, you eat an elephant with small bites, so I think fixing these directly for now would probably be the quickest path to resolution. This would also ease abstraction later, because it would add sensible checks at the same location where abstraction would need to test for query results anyway.