[aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid

[elij]

On Wed, May 11, 2011 at 5:09 PM, Lukas Fleischer
<archlinux at 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.