Re: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid
On Wed, May 11, 2011 at 5:09 PM, Lukas Fleischer
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.
Re: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid
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? 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".
Re: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid
hmm. looks like there were still a couple lines of formatting junk in this patch.. hilariously the ones I missed are converting spaces to tabs to be more consistent. &_& the count is low though. Skimming the patch file it looks like only 3 or 4 lines. I got the big ones pruned out of the patch though.
[aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid
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(-)
diff --git a/web/html/addvote.php b/web/html/addvote.php
index 5936d56..a459610 100644
--- a/web/html/addvote.php
+++ b/web/html/addvote.php
@@ -21,14 +21,26 @@ if ($atype == "Trusted User" OR $atype == "Developer") {
if (!empty($_POST['user'])) {
$qcheck = "SELECT * FROM Users WHERE Username = '" .
mysql_real_escape_string($_POST['user']) . "'";
- $check = mysql_num_rows(db_query($qcheck, $dbh));
+ $result = db_query($qcheck, $dbh);
+ if ($result) {
+ $check = mysql_num_rows($result);
+ }
+ else {
+ $check = 0;
+ }
if ($check == 0) {
$error.= __("Username does not exist.");
} else {
$qcheck = "SELECT * FROM TU_VoteInfo WHERE User
= '" . mysql_real_escape_string($_POST['user']) . "'";
$qcheck.= " AND End > UNIX_TIMESTAMP()";
- $check = mysql_num_rows(db_query($qcheck,
$dbh));
+ $result = db_query($qcheck, $dbh);
+ if ($result) {
+ $check = mysql_num_rows($result);
+ }
+ else {
+ $check = 0;
+ }
if ($check != 0) {
$error.= __("%s already has proposal
running for them.", htmlentities($_POST['user']));
diff --git a/web/html/tu.php b/web/html/tu.php
index c5cc36b..6ab8ae9 100644
--- a/web/html/tu.php
+++ b/web/html/tu.php
@@ -36,7 +36,13 @@ if ($atype == "Trusted User" OR $atype == "Developer") {
$qvoted = "SELECT * FROM TU_Votes WHERE ";
$qvoted.= "VoteID = " . $row['ID'] . " AND ";
$qvoted.= "UserID = " .
uid_from_sid($_COOKIE["AURSID"]);
- $hasvoted = mysql_num_rows(db_query($qvoted,
$dbh));
+ $result = db_query($qvoted, $dbh);
+ if ($result) {
+ $hasvoted = mysql_num_rows($result);
+ }
+ else {
+ $hasvoted = 0;
+ }
# List voters of a proposal.
$qwhoVoted = "SELECT tv.UserID,U.Username
@@ -85,10 +91,15 @@ if ($atype == "Trusted User" OR $atype == "Developer") {
$canvote = 0;
$errorvote = __("You've already
voted for this proposal.");
# Update if they voted
- $hasvoted =
mysql_num_rows(db_query($qvoted, $dbh));
+ $result = db_query($qvoted,
$dbh);
+ if ($result) {
+ $hasvoted =
mysql_num_rows($result);
+ }
$results = db_query($q, $dbh);
- $row =
mysql_fetch_assoc($results);
+ if ($results) {
+ $row =
mysql_fetch_assoc($results);
+ }
}
}
include("tu_details.php");
diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc
index 8ffa2f7..5bcff8b 100644
--- a/web/lib/acctfuncs.inc
+++ b/web/lib/acctfuncs.inc
@@ -197,7 +197,7 @@ function
process_account_form($UTYPE,$TYPE,$A,$U="",$T="",$S="",$E="",
}
if (!$error && !valid_username($U) && !user_is_privileged($editor_user))
-$error = __("The username is invalid.") . "\n"
+ $error = __("The
Re: [aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid
On Tue, May 10, 2011 at 09:01:28PM -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 | 116 + > web/template/actions_form.php| 52 ++ > web/template/pkg_search_form.php |2 +- > web/template/tu_list.php |8 ++- > 8 files changed, 172 insertions(+), 106 deletions(-) > Yeah, cool. I guess there's some more places that need to get fixed but that looks like a good start. Could you please resend that one without the formatting changes? They make it kinda hard to spot the actual changes.
[aur-dev] [PATCH 2/4] test return value from db_query before assuming it is valid
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 | 116 +
web/template/actions_form.php| 52 ++
web/template/pkg_search_form.php |2 +-
web/template/tu_list.php |8 ++-
8 files changed, 172 insertions(+), 106 deletions(-)
diff --git a/web/html/addvote.php b/web/html/addvote.php
index 5936d56..a459610 100644
--- a/web/html/addvote.php
+++ b/web/html/addvote.php
@@ -21,14 +21,26 @@ if ($atype == "Trusted User" OR $atype == "Developer") {
if (!empty($_POST['user'])) {
$qcheck = "SELECT * FROM Users WHERE Username = '" .
mysql_real_escape_string($_POST['user']) . "'";
- $check = mysql_num_rows(db_query($qcheck, $dbh));
+ $result = db_query($qcheck, $dbh);
+ if ($result) {
+ $check = mysql_num_rows($result);
+ }
+ else {
+ $check = 0;
+ }
if ($check == 0) {
$error.= __("Username does not exist.");
} else {
$qcheck = "SELECT * FROM TU_VoteInfo WHERE User
= '" . mysql_real_escape_string($_POST['user']) . "'";
$qcheck.= " AND End > UNIX_TIMESTAMP()";
- $check = mysql_num_rows(db_query($qcheck,
$dbh));
+ $result = db_query($qcheck, $dbh);
+ if ($result) {
+ $check = mysql_num_rows($result);
+ }
+ else {
+ $check = 0;
+ }
if ($check != 0) {
$error.= __("%s already has proposal
running for them.", htmlentities($_POST['user']));
diff --git a/web/html/tu.php b/web/html/tu.php
index c5cc36b..6ab8ae9 100644
--- a/web/html/tu.php
+++ b/web/html/tu.php
@@ -36,7 +36,13 @@ if ($atype == "Trusted User" OR $atype == "Developer") {
$qvoted = "SELECT * FROM TU_Votes WHERE ";
$qvoted.= "VoteID = " . $row['ID'] . " AND ";
$qvoted.= "UserID = " .
uid_from_sid($_COOKIE["AURSID"]);
- $hasvoted = mysql_num_rows(db_query($qvoted,
$dbh));
+ $result = db_query($qvoted, $dbh);
+ if ($result) {
+ $hasvoted = mysql_num_rows($result);
+ }
+ else {
+ $hasvoted = 0;
+ }
# List voters of a proposal.
$qwhoVoted = "SELECT tv.UserID,U.Username
@@ -85,10 +91,15 @@ if ($atype == "Trusted User" OR $atype == "Developer") {
$canvote = 0;
$errorvote = __("You've already
voted for this proposal.");
# Update if they voted
- $hasvoted =
mysql_num_rows(db_query($qvoted, $dbh));
+ $result = db_query($qvoted,
$dbh);
+ if ($result) {
+ $hasvoted =
mysql_num_rows($result);
+ }
$results = db_query($q, $dbh);
- $row =
mysql_fetch_assoc($results);
+ if ($results) {
+ $row =
mysql_fetch_assoc($results);
+ }
}
}
include("tu_details.php");
diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc
index 8ffa2f7..5bcff8b 100644
--- a/web/lib/acctfuncs.inc
+++ b/web/lib/acctfuncs.inc
@@ -197,7 +197,7 @@ function
process_account_form($UTYPE,$TYPE,$A,$U="",$T="",$S="",$E="",
}
if (!$error && !valid_username($U) && !user_is_privileged($editor_user))
-$error = __("The username is invalid.") . "\n"
+
