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 1/4] remove submitter from package data
On Wed, May 11, 2011 at 12:00:50PM -0700, elij wrote: > On Wed, May 11, 2011 at 11:51 AM, Dan McGee wrote: > > On Wed, May 11, 2011 at 1:48 PM, elij wrote: > >> On Wed, May 11, 2011 at 7:11 AM, Lukas Fleischer > >> wrote: > >>> On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote: > --- > web/html/pkgsubmit.php | 3 +-- > web/lib/pkgfuncs.inc | 10 +- > web/template/pkg_details.php | 11 --- > 3 files changed, 2 insertions(+), 22 deletions(-) > > >>> > >>> The submitter field proved to be useful in some cases where a package > >>> was moved from the official repos to the AUR and either turned out to be > >>> incomplete or wasn't properly removed from the official repos. > >> > >> I guess I don't see what benefit the submitter field would have in > >> such an instance. > >> If someone moved it from the official repos to the aur, would they not > >> be the submitter and also the maintainer? > > Initially, yes. And then we all usually orphan the junk because we > > don't want it, we just put it there for postarity, so you've > > immediately lost information. > > > > I think it has a lot less usefulness on the web page itself (at least > > for the general public), so I wouldn't be against culling it there, > > but as far as a point of reference when trying to look at the > > packages, it makes since to keep around. It can be far different than > > what the maintainer field tells you. > > Hmm. So keeping it but maybe only showing it to TU or Developer class > users may be more appropriate. Agreed. > Alternatively, it almost sounds like maintainer and submitter could be > merged into an 'owner' value, and track owner history somehow (record > each change of ownership in a join table). That might add the ability > to track users that upload lots of packages, then disown them too. And > track packages with high owner turnover (may tell whether a package is > painful or burdensome to maintain). Hm, sounds cool, but I'm not sure if it's worth implementing. I don't see any real benefit from the feature itself or from the statistics that could be created using this yet. > > If such a thing were implemented though, I think it should be > displayed only to TU or Developer class users. I can't see general > users finding much use for it, but I could be wrong.
Re: [aur-dev] [PATCH 3/4] fix case where user does not exist
On Wed, May 11, 2011 at 11:54:34AM -0700, elij wrote:
> On Wed, May 11, 2011 at 7:22 AM, Lukas Fleischer
> wrote:
> > On Tue, May 10, 2011 at 09:01:29PM -0700, elij wrote:
> >> the query was being performed when $id was not set, resulting in an
> >> invalid sql query being performed.
> >> ---
> >> web/lib/acctfuncs.inc | 3 +++
> >> 1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc
> >> index 5bcff8b..b2f0548 100644
> >> --- a/web/lib/acctfuncs.inc
> >> +++ b/web/lib/acctfuncs.inc
> >> @@ -786,6 +786,9 @@ function valid_passwd( $userID, $passwd )
> >> */
> >> function user_suspended( $id )
> >> {
> >> + if (!$id) {
> >> + return false;
> >> + }
> >> $dbh = db_connect();
> >> $q = "SELECT Suspended FROM Users WHERE ID = " . $id;
> >> $result = db_query($q, $dbh);
> >
> > Looks ok, but I'd rather say we should locate the code path that led to
> > the unset parameter and add some additional validation there to avoid
> > further unexpected behaviour.
>
> The source is in try_login (also in lib/acctfuncs.inc):
>
> if ( isset($_REQUEST['user']) || isset($_REQUEST['passwd']) ) {
> $userID = valid_user($_REQUEST['user']);
> if ( user_suspended( $userID ) ) {
> $login_error = "Account Suspended.";
> }
Thanks. I will look into that.
> valid_user (also in the same file) can return (no value) in some cases.
> that large if/elseif block in try_login could probably have some more
> conditions added to test for existence of $userID before calling
> user_suspended on it. Still.. it might make sense to have a guard (the
> test added with this patch) in the function itself in case usage of it
> down the road changes (another code path or something).
Yeah, will be pushed.
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 1/4] remove submitter from package data
On Wed, May 11, 2011 at 11:51 AM, Dan McGee wrote: > On Wed, May 11, 2011 at 1:48 PM, elij wrote: >> On Wed, May 11, 2011 at 7:11 AM, Lukas Fleischer >> wrote: >>> On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote: --- web/html/pkgsubmit.php | 3 +-- web/lib/pkgfuncs.inc | 10 +- web/template/pkg_details.php | 11 --- 3 files changed, 2 insertions(+), 22 deletions(-) >>> >>> The submitter field proved to be useful in some cases where a package >>> was moved from the official repos to the AUR and either turned out to be >>> incomplete or wasn't properly removed from the official repos. >> >> I guess I don't see what benefit the submitter field would have in >> such an instance. >> If someone moved it from the official repos to the aur, would they not >> be the submitter and also the maintainer? > Initially, yes. And then we all usually orphan the junk because we > don't want it, we just put it there for postarity, so you've > immediately lost information. > > I think it has a lot less usefulness on the web page itself (at least > for the general public), so I wouldn't be against culling it there, > but as far as a point of reference when trying to look at the > packages, it makes since to keep around. It can be far different than > what the maintainer field tells you. Hmm. So keeping it but maybe only showing it to TU or Developer class users may be more appropriate. Alternatively, it almost sounds like maintainer and submitter could be merged into an 'owner' value, and track owner history somehow (record each change of ownership in a join table). That might add the ability to track users that upload lots of packages, then disown them too. And track packages with high owner turnover (may tell whether a package is painful or burdensome to maintain). If such a thing were implemented though, I think it should be displayed only to TU or Developer class users. I can't see general users finding much use for it, but I could be wrong.
Re: [aur-dev] [PATCH 3/4] fix case where user does not exist
On Wed, May 11, 2011 at 7:22 AM, Lukas Fleischer
wrote:
> On Tue, May 10, 2011 at 09:01:29PM -0700, elij wrote:
>> the query was being performed when $id was not set, resulting in an
>> invalid sql query being performed.
>> ---
>> web/lib/acctfuncs.inc | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc
>> index 5bcff8b..b2f0548 100644
>> --- a/web/lib/acctfuncs.inc
>> +++ b/web/lib/acctfuncs.inc
>> @@ -786,6 +786,9 @@ function valid_passwd( $userID, $passwd )
>> */
>> function user_suspended( $id )
>> {
>> + if (!$id) {
>> + return false;
>> + }
>> $dbh = db_connect();
>> $q = "SELECT Suspended FROM Users WHERE ID = " . $id;
>> $result = db_query($q, $dbh);
>
> Looks ok, but I'd rather say we should locate the code path that led to
> the unset parameter and add some additional validation there to avoid
> further unexpected behaviour.
The source is in try_login (also in lib/acctfuncs.inc):
if ( isset($_REQUEST['user']) || isset($_REQUEST['passwd']) ) {
$userID = valid_user($_REQUEST['user']);
if ( user_suspended( $userID ) ) {
$login_error = "Account Suspended.";
}
valid_user (also in the same file) can return (no value) in some cases.
that large if/elseif block in try_login could probably have some more
conditions added to test for existence of $userID before calling
user_suspended on it. Still.. it might make sense to have a guard (the
test added with this patch) in the function itself in case usage of it
down the road changes (another code path or something).
Re: [aur-dev] [PATCH 1/4] remove submitter from package data
On Wed, May 11, 2011 at 1:48 PM, elij wrote: > On Wed, May 11, 2011 at 7:11 AM, Lukas Fleischer > wrote: >> On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote: >>> --- >>> web/html/pkgsubmit.php | 3 +-- >>> web/lib/pkgfuncs.inc | 10 +- >>> web/template/pkg_details.php | 11 --- >>> 3 files changed, 2 insertions(+), 22 deletions(-) >>> >> >> The submitter field proved to be useful in some cases where a package >> was moved from the official repos to the AUR and either turned out to be >> incomplete or wasn't properly removed from the official repos. > > I guess I don't see what benefit the submitter field would have in > such an instance. > If someone moved it from the official repos to the aur, would they not > be the submitter and also the maintainer? Initially, yes. And then we all usually orphan the junk because we don't want it, we just put it there for postarity, so you've immediately lost information. I think it has a lot less usefulness on the web page itself (at least for the general public), so I wouldn't be against culling it there, but as far as a point of reference when trying to look at the packages, it makes since to keep around. It can be far different than what the maintainer field tells you. -Dan
Re: [aur-dev] [PATCH 1/4] remove submitter from package data
On Wed, May 11, 2011 at 7:11 AM, Lukas Fleischer wrote: > On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote: >> --- >> web/html/pkgsubmit.php | 3 +-- >> web/lib/pkgfuncs.inc | 10 +- >> web/template/pkg_details.php | 11 --- >> 3 files changed, 2 insertions(+), 22 deletions(-) >> > > The submitter field proved to be useful in some cases where a package > was moved from the official repos to the AUR and either turned out to be > incomplete or wasn't properly removed from the official repos. I guess I don't see what benefit the submitter field would have in such an instance. If someone moved it from the official repos to the aur, would they not be the submitter and also the maintainer? > What's the motivation of removing this one? There was a discussion in another thread about adding submitter to the api return results, and I mentioned that I felt that submitter field wasn't very relevant or useful. This patch stemmed from that discussion.
Re: [aur-dev] [PATCH 4/4] make return value consistent for this function
On Tue, May 10, 2011 at 09:01:30PM -0700, elij wrote:
> ---
> web/lib/aur.inc |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/web/lib/aur.inc b/web/lib/aur.inc
> index fb267af..f8fa715 100644
> --- a/web/lib/aur.inc
> +++ b/web/lib/aur.inc
> @@ -196,7 +196,7 @@ function uid_from_sid($sid="") {
> $q.= "AND Sessions.SessionID = '" . mysql_real_escape_string($sid) .
> "'";
> $result = db_query($q, $dbh);
> if (!$result) {
> - return 0;
> + return "";
> }
> $row = mysql_fetch_row($result);
>
Yes, that is one of the code parts that really suck. Returning zero or
an empty string in a function that is called uid_from_sid() is just
nonsense. I started refactoring this, replacing all zeros and empty
strings in functions that are designed to return IDs by "null" but
didn't ever finish that since I didn't have enough time to check and fix
all invocations, also (some of them compare to "0" and stuff).
It would be cool to have someone looking into that :)
Re: [aur-dev] [PATCH 3/4] fix case where user does not exist
On Tue, May 10, 2011 at 09:01:29PM -0700, elij wrote:
> the query was being performed when $id was not set, resulting in an
> invalid sql query being performed.
> ---
> web/lib/acctfuncs.inc |3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc
> index 5bcff8b..b2f0548 100644
> --- a/web/lib/acctfuncs.inc
> +++ b/web/lib/acctfuncs.inc
> @@ -786,6 +786,9 @@ function valid_passwd( $userID, $passwd )
> */
> function user_suspended( $id )
> {
> + if (!$id) {
> + return false;
> + }
> $dbh = db_connect();
> $q = "SELECT Suspended FROM Users WHERE ID = " . $id;
> $result = db_query($q, $dbh);
Looks ok, but I'd rather say we should locate the code path that led to
the unset parameter and add some additional validation there to avoid
further unexpected behaviour.
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.
Re: [aur-dev] [PATCH 1/4] remove submitter from package data
On Tue, May 10, 2011 at 09:01:27PM -0700, elij wrote: > --- > web/html/pkgsubmit.php |3 +-- > web/lib/pkgfuncs.inc | 10 +- > web/template/pkg_details.php | 11 --- > 3 files changed, 2 insertions(+), 22 deletions(-) > The submitter field proved to be useful in some cases where a package was moved from the official repos to the AUR and either turned out to be incomplete or wasn't properly removed from the official repos. What's the motivation of removing this one?
