[aur-dev] some notes from converting the aur from mysql to postgresql

elij

26 Apr 2011 26 Apr '11

4:35 a.m.

While converting the aur codebase from mysql to postgresql (just for fun/to see if I could), I ran into a few mysql specific gotchas, and a few oddities. Here are some of my notes from the experience. * I ran into a few sql integer comparisons that are using string comparisons. postgres didn't like this, but mysql was fine with it ex: `$q .= "WHERE s.UsersId = '" . $userID . "' ";` * using the limit offset format of `"LIMIT x,y"` postgresql doesn't support this format, so I had to track each occurrence down and change it to `"Limit x OFFSET y"` * A few instances of `"LIMIT 0,10"`. A `"LIMIT 10"` would have been sufficient. Not sure why offset was specified in these few instances. * A pair of column names were reserved words. "user' and 'end' in the TU_VoteInfo table. I changed the column names to username and ending, to avoid issues and/or having to quote the name all the time. * postgresql table and column names are lowercase (when not quoted). This required me to either quote all instances of table and column names (ugh) or change the fetch_assoc php references to lowercase. I chose the latter, even though it was tedious to track down all instances and change them. * postgres has no mysql_insert_id counterpart. Instead I appended to the query "RETURNING ID" and then used pg_fetch_result to get the ID that was created. * postgres LIKE and ILIKE queries did full table scans, and was quite slow. I think mysql was caching these queries when I was doing my performance tests. The end result was postgres performing poorly in my load test for LIKE/ILIKE operations. So I created a postgres fulltext search column for the packages table, indexed it, defined triggers to keep it udpated, and converted to using postgres fulltext searching `"@@ to_tsquery('term')"`. The result was quite speedy and gave nice results. The syntax was a bit different for 'prefix searching' though, and the results were not the same as the LIKE/ILIKE queries, because fulltext uses stemming instead of partial string comparison. * There is quite a bit of sql code sprinkled all over the place. It might make sense to try and consolidate some of the sql to fewer files. * It might make sense to generalize the 'mysql_real_escape_string' instances to a wrapper 'db_escape' or something similar. This would could make things a bit cleaner, ease possible future porting to other databases (fewer things to track down and change individually). * postgres doesn't support `"UNIX_TIMESTAMP()"`. Instead of changing all instances of it, I defined a few convenience sql functions. -- -- helper functions to ease conversion from mysql -- from: http://janusz.slota.name/blog/2009/06/mysql-from_unixtime-and-unix_timestamp... -- -- no params CREATE OR REPLACE FUNCTION unix_timestamp() RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM CURRENT_TIMESTAMP(0))::bigint AS result; ' LANGUAGE 'SQL'; -- timestamp without time zone (i.e. 1973-11-29 21:33:09) CREATE OR REPLACE FUNCTION unix_timestamp(TIMESTAMP) RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM $1)::bigint AS result; ' LANGUAGE 'SQL'; -- timestamp with time zone (i.e. 1973-11-29 21:33:09+01) CREATE OR REPLACE FUNCTION unix_timestamp(TIMESTAMP WITH TIME zone) RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM $1)::bigint AS result; ' LANGUAGE 'SQL'; I think that is it.

Show replies by date

Dan McGee

26 Apr 26 Apr

4:49 a.m.

On Mon, Apr 25, 2011 at 9:35 PM, elij wrote:

...

While converting the aur codebase from mysql to postgresql (just for fun/to see if I could), I ran into a few mysql specific gotchas, and a few oddities. Here are some of my notes from the experience.

Mind sending the diff along for funzies? Even if we can't apply it as a patch, it would be good to see and maybe chop up into "this will be compatible with all DBs so we should fix it anyway" and "if we switch we will need to look into this".

...

* I ran into a few sql integer comparisons that are using string comparisons. postgres didn't like this, but mysql was fine with it ex: `$q .= "WHERE s.UsersId = '" . $userID . "' ";` Should definitely be fixed anyway...awesome.

...

* using the limit offset format of `"LIMIT x,y"` postgresql doesn't support this format, so I had to track each occurrence down and change it to `"Limit x OFFSET y"` This is SQL standard, so you could use this form anyway, correct? Of course the MySQL docs claim this: For compatibility with PostgreSQL, MySQL also supports the LIMIT row_count OFFSET offset syntax.

...

* A few instances of `"LIMIT 0,10"`. A `"LIMIT 10"` would have been sufficient. Not sure why offset was specified in these few instances. Sure it wasn't auto-generated, in which case it is easier for the DB query analyzer to just throw it away anyway?

...

* A pair of column names were reserved words. "user' and 'end' in the TU_VoteInfo table. I changed the column names to username and ending, to avoid issues and/or having to quote the name all the time.

* postgresql table and column names are lowercase (when not quoted). This required me to either quote all instances of table and column names (ugh) or change the fetch_assoc php references to lowercase. I chose the latter, even though it was tedious to track down all instances and change them.

* postgres has no mysql_insert_id counterpart. Instead I appended to the query "RETURNING ID" and then used pg_fetch_result to get the ID that was created.

* postgres LIKE and ILIKE queries did full table scans, and was quite slow. I think mysql was caching these queries when I was doing my performance tests. The end result was postgres performing poorly in my load test for LIKE/ILIKE operations. So I created a postgres fulltext search column for the packages table, indexed it, defined triggers to keep it udpated, and converted to using postgres fulltext searching `"@@ to_tsquery('term')"`. The result was quite speedy and gave nice results. The syntax was a bit different for 'prefix searching' though, and the results were not the same as the LIKE/ILIKE queries, because fulltext uses stemming instead of partial string comparison. And case sensitivity would come into play, no?

...

* There is quite a bit of sql code sprinkled all over the place. It might make sense to try and consolidate some of the sql to fewer files.

* It might make sense to generalize the 'mysql_real_escape_string' instances to a wrapper 'db_escape' or something similar. This would could make things a bit cleaner, ease possible future porting to other databases (fewer things to track down and change individually).

* postgres doesn't support `"UNIX_TIMESTAMP()"`. Instead of changing all instances of it, I defined a few convenience sql functions.

-- -- helper functions to ease conversion from mysql -- from: http://janusz.slota.name/blog/2009/06/mysql-from_unixtime-and-unix_timestamp... -- -- no params CREATE OR REPLACE FUNCTION unix_timestamp() RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM CURRENT_TIMESTAMP(0))::bigint AS result; ' LANGUAGE 'SQL';

-- timestamp without time zone (i.e. 1973-11-29 21:33:09) CREATE OR REPLACE FUNCTION unix_timestamp(TIMESTAMP) RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM $1)::bigint AS result; ' LANGUAGE 'SQL';

-- timestamp with time zone (i.e. 1973-11-29 21:33:09+01) CREATE OR REPLACE FUNCTION unix_timestamp(TIMESTAMP WITH TIME zone) RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM $1)::bigint AS result; ' LANGUAGE 'SQL';

I think that is it.

elij

5:38 a.m.

On Mon, Apr 25, 2011 at 7:49 PM, Dan McGee wrote:

...

On Mon, Apr 25, 2011 at 9:35 PM, elij wrote:

...
While converting the aur codebase from mysql to postgresql (just for fun/to see if I could), I ran into a few mysql specific gotchas, and a few oddities. Here are some of my notes from the experience.

Mind sending the diff along for funzies? Even if we can't apply it as a patch, it would be good to see and maybe chop up into "this will be compatible with all DBs so we should fix it anyway" and "if we switch we will need to look into this".

Sure. I will probably just do a git-format patch and dump the patches into a tarball and upload it somewhere, since it is quite large, and not exactly 'split into single change patches'. ;)

...

...
* I ran into a few sql integer comparisons that are using string comparisons. postgres didn't like this, but mysql was fine with it ex: `$q .= "WHERE s.UsersId = '" . $userID . "' ";` Should definitely be fixed anyway...awesome.

Yeah. Some of these fixes in the above patchset are wrapped up in other things, but I can try to split out the "these can be fixed regardless" and "there were fixes to get postgres working".

...

...
* using the limit offset format of `"LIMIT x,y"` postgresql doesn't support this format, so I had to track each occurrence down and change it to `"Limit x OFFSET y"` This is SQL standard, so you could use this form anyway, correct? Of course the MySQL docs claim this: For compatibility with PostgreSQL, MySQL also supports the LIMIT row_count OFFSET offset syntax.

Yeah. LIMIT w/OFFSET is sql standard. Not sure why mysql had to create their own limit/offset standard, but well.. yeah.

...

...
* A few instances of `"LIMIT 0,10"`. A `"LIMIT 10"` would have been sufficient. Not sure why offset was specified in these few instances. Sure it wasn't auto-generated, in which case it is easier for the DB query analyzer to just throw it away anyway?

It looks like there were just two instances. web/html/rss.php: $q = "SELECT * FROM Packages "; $q.= "WHERE DummyPkg != 1 "; $q.= "ORDER BY SubmittedTS DESC "; $q.= "LIMIT 0 , 20"; web/lib/stats.inc: $q = 'SELECT * FROM Packages WHERE DummyPkg != 1 ORDER BY GREATEST(SubmittedTS,ModifiedTS) DESC LIMIT 0 , 10';

...

...
* A pair of column names were reserved words. "user' and 'end' in the TU_VoteInfo table. I changed the column names to username and ending, to avoid issues and/or having to quote the name all the time.

* postgresql table and column names are lowercase (when not quoted). This required me to either quote all instances of table and column names (ugh) or change the fetch_assoc php references to lowercase. I chose the latter, even though it was tedious to track down all instances and change them.

* postgres has no mysql_insert_id counterpart. Instead I appended to the query "RETURNING ID" and then used pg_fetch_result to get the ID that was created.

* postgres LIKE and ILIKE queries did full table scans, and was quite slow. I think mysql was caching these queries when I was doing my performance tests. The end result was postgres performing poorly in my load test for LIKE/ILIKE operations. So I created a postgres fulltext search column for the packages table, indexed it, defined triggers to keep it udpated, and converted to using postgres fulltext searching `"@@ to_tsquery('term')"`. The result was quite speedy and gave nice results. The syntax was a bit different for 'prefix searching' though, and the results were not the same as the LIKE/ILIKE queries, because fulltext uses stemming instead of partial string comparison. And case sensitivity would come into play, no?

I built the fulltext index as follows: alter table packages add column tsv tsvector; update packages set tsv = to_tsvector('english', coalesce(lower(name), '') || ' ' || coalesce(lower(description), '')); then added a trigger to update it for new data: DROP FUNCTION IF EXISTS package_fulltext_trigger() CASCADE; CREATE FUNCTION package_fulltext_trigger() RETURNS trigger as $$ begin new.tsv := setweight(to_tsvector('english', coalesce(lower(new.name), '')), 'A') || setweight(to_tsvector('english', coalesce(lower(new.description), '')), 'B'); return new; end $$ LANGUAGE plpgsql; DROP TRIGGER IF EXISTS packages_tsv_update; CREATE TRIGGER packages_tsv_update BEFORE INSERT OR UPDATE on packages FOR EACH ROW EXECUTE PROCEDURE package_fulltext_trigger();

...

...
* There is quite a bit of sql code sprinkled all over the place. It might make sense to try and consolidate some of the sql to fewer files.

* It might make sense to generalize the 'mysql_real_escape_string' instances to a wrapper 'db_escape' or something similar. This would could make things a bit cleaner, ease possible future porting to other databases (fewer things to track down and change individually).

* postgres doesn't support `"UNIX_TIMESTAMP()"`. Instead of changing all instances of it, I defined a few convenience sql functions.

-- -- helper functions to ease conversion from mysql -- from: http://janusz.slota.name/blog/2009/06/mysql-from_unixtime-and-unix_timestamp... -- -- no params CREATE OR REPLACE FUNCTION unix_timestamp() RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM CURRENT_TIMESTAMP(0))::bigint AS result; ' LANGUAGE 'SQL';

-- timestamp without time zone (i.e. 1973-11-29 21:33:09) CREATE OR REPLACE FUNCTION unix_timestamp(TIMESTAMP) RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM $1)::bigint AS result; ' LANGUAGE 'SQL';

-- timestamp with time zone (i.e. 1973-11-29 21:33:09+01) CREATE OR REPLACE FUNCTION unix_timestamp(TIMESTAMP WITH TIME zone) RETURNS BIGINT AS ' SELECT EXTRACT(EPOCH FROM $1)::bigint AS result; ' LANGUAGE 'SQL';

I think that is it.

elij

5:40 a.m.

On Mon, Apr 25, 2011 at 8:38 PM, elij wrote:

...

On Mon, Apr 25, 2011 at 7:49 PM, Dan McGee wrote:

...
And case sensitivity would come into play, no?

I built the fulltext index as follows: alter table packages add column tsv tsvector; update packages set tsv = to_tsvector('english', coalesce(lower(name), '') || ' ' || coalesce(lower(description), ''));

then added a trigger to update it for new data:

DROP FUNCTION IF EXISTS package_fulltext_trigger() CASCADE; CREATE FUNCTION package_fulltext_trigger() RETURNS trigger as $$ begin new.tsv := setweight(to_tsvector('english', coalesce(lower(new.name), '')), 'A') || setweight(to_tsvector('english', coalesce(lower(new.description), '')), 'B'); return new; end $$ LANGUAGE plpgsql;

DROP TRIGGER IF EXISTS packages_tsv_update; CREATE TRIGGER packages_tsv_update BEFORE INSERT OR UPDATE on packages FOR EACH ROW EXECUTE PROCEDURE package_fulltext_trigger();

oops. Forgot to complete that thought.. "thus, it is case insensitive because everything is lowercased before turning it into a fulltext tsvector. The search input would probably need to be lowered as well, for the above implementation."

Dan McGee

5:50 a.m.

On Mon, Apr 25, 2011 at 10:38 PM, elij wrote:

...

On Mon, Apr 25, 2011 at 7:49 PM, Dan McGee wrote:

...
On Mon, Apr 25, 2011 at 9:35 PM, elij wrote:

...
* using the limit offset format of `"LIMIT x,y"` postgresql doesn't support this format, so I had to track each occurrence down and change it to `"Limit x OFFSET y"` This is SQL standard, so you could use this form anyway, correct? Of course the MySQL docs claim this: For compatibility with PostgreSQL, MySQL also supports the LIMIT row_count OFFSET offset syntax.

Yeah. LIMIT w/OFFSET is sql standard. Not sure why mysql had to create their own limit/offset standard, but well.. yeah.

...
...
* A few instances of `"LIMIT 0,10"`. A `"LIMIT 10"` would have been sufficient. Not sure why offset was specified in these few instances. Sure it wasn't auto-generated, in which case it is easier for the DB query analyzer to just throw it away anyway?

It looks like there were just two instances.

web/html/rss.php: $q = "SELECT * FROM Packages "; $q.= "WHERE DummyPkg != 1 "; $q.= "ORDER BY SubmittedTS DESC "; $q.= "LIMIT 0 , 20";

web/lib/stats.inc: $q = 'SELECT * FROM Packages WHERE DummyPkg != 1 ORDER BY GREATEST(SubmittedTS,ModifiedTS) DESC LIMIT 0 , 10';

If you still have DummyPkg references, you might need to update your codebase- this was killed a while ago by me. With that said, the first one still exists; the second one was fixed in the aforementioned update. -Dan

elij

5:52 a.m.

On Mon, Apr 25, 2011 at 8:50 PM, Dan McGee wrote:

...

On Mon, Apr 25, 2011 at 10:38 PM, elij wrote:

...
On Mon, Apr 25, 2011 at 7:49 PM, Dan McGee wrote:

...
On Mon, Apr 25, 2011 at 9:35 PM, elij wrote:

...
* using the limit offset format of `"LIMIT x,y"` postgresql doesn't support this format, so I had to track each occurrence down and change it to `"Limit x OFFSET y"` This is SQL standard, so you could use this form anyway, correct? Of course the MySQL docs claim this: For compatibility with PostgreSQL, MySQL also supports the LIMIT row_count OFFSET offset syntax.

Yeah. LIMIT w/OFFSET is sql standard. Not sure why mysql had to create their own limit/offset standard, but well.. yeah.

...
...
* A few instances of `"LIMIT 0,10"`. A `"LIMIT 10"` would have been sufficient. Not sure why offset was specified in these few instances. Sure it wasn't auto-generated, in which case it is easier for the DB query analyzer to just throw it away anyway?

It looks like there were just two instances.

web/html/rss.php: $q = "SELECT * FROM Packages "; $q.= "WHERE DummyPkg != 1 "; $q.= "ORDER BY SubmittedTS DESC "; $q.= "LIMIT 0 , 20";

web/lib/stats.inc: $q = 'SELECT * FROM Packages WHERE DummyPkg != 1 ORDER BY GREATEST(SubmittedTS,ModifiedTS) DESC LIMIT 0 , 10';

If you still have DummyPkg references, you might need to update your codebase- this was killed a while ago by me.

With that said, the first one still exists; the second one was fixed in the aforementioned update.

Ah. Yeah, I probably do need to do a pull and merge (been a while). Thanks for the heads up.

Dan McGee

6:23 a.m.

New subject: [aur-dev] [PATCH 1/3] SQL: use standard LIMIT/OFFSET syntax

Increases compatibility with standard SQL dialect. Thanks-to: elij Signed-off-by: Dan McGee --- Smoke tested locally: * rss.php still worked * tu.php still worked when clicking "Next" * account.php still worked when clicking "More" (wtf "Less" and "More"?) * packages.php still worked when clicking "Next" web/html/rss.php | 2 +- web/html/tu.php | 2 +- web/lib/acctfuncs.inc | 2 +- web/lib/pkgfuncs.inc | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/web/html/rss.php b/web/html/rss.php index cb0bf40..0547815 100644 --- a/web/html/rss.php +++ b/web/html/rss.php @@ -32,7 +32,7 @@ $rss->image = $image; $dbh = db_connect(); $q = "SELECT * FROM Packages "; $q.= "ORDER BY SubmittedTS DESC "; -$q.= "LIMIT 0 , 20"; +$q.= "LIMIT 20"; $result = db_query($q, $dbh); while ($row = mysql_fetch_assoc($result)) { diff --git a/web/html/tu.php b/web/html/tu.php index 941e6ed..dc1c5e3 100644 --- a/web/html/tu.php +++ b/web/html/tu.php @@ -119,7 +119,7 @@ if ($atype == "Trusted User" OR $atype == "Developer") { } $order = ($by == 'asc') ? 'ASC' : 'DESC'; - $lim = ($limit > 0) ? " LIMIT $off, $limit" : ""; + $lim = ($limit > 0) ? " LIMIT $limit OFFSET $off" : ""; $by_next = ($by == 'desc') ? 'asc' : 'desc'; $q = "SELECT * FROM TU_VoteInfo WHERE End > " . time() . " ORDER BY Submitted " . $order; diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc index fe1cfb1..8e2ecb3 100644 --- a/web/lib/acctfuncs.inc +++ b/web/lib/acctfuncs.inc @@ -405,7 +405,7 @@ function search_results_page($UTYPE,$O=0,$SB="",$U="",$T="", break; } $search_vars[] = "SB"; - $q.= "LIMIT ". $OFFSET . ", " . $HITS_PER_PAGE; + $q.= "LIMIT " . $HITS_PER_PAGE . " OFFSET " . $OFFSET; $dbh = db_connect(); diff --git a/web/lib/pkgfuncs.inc b/web/lib/pkgfuncs.inc index 8f90d0e..7b43e45 100644 --- a/web/lib/pkgfuncs.inc +++ b/web/lib/pkgfuncs.inc @@ -501,7 +501,7 @@ function pkg_search_page($SID="") { break; } - $q_limit = "LIMIT ".$_GET["O"].", ".$_GET["PP"]; + $q_limit = "LIMIT ".$_GET["PP"]." OFFSET ".$_GET["O"]; $q = $q_select . $q_from . $q_from_extra . $q_where . $q_sort . $q_limit; $q_total = "SELECT COUNT(*) " . $q_from . $q_where; -- 1.7.5

Dan McGee

6:23 a.m.

New subject: [aur-dev] [PATCH 2/3] SQL: treat all UID/ID values as numbers, not strings

Ensure we are not quoting these values in any of our SQL queries. Thanks-to: elij Signed-off-by: Dan McGee --- Smoke tested: * Submitted a package update and it worked * Updated a user's password from non-salted to salted variety * Numerous login/logout cycles web/html/passreset.php | 4 ++-- web/html/pkgsubmit.php | 2 +- web/lib/acctfuncs.inc | 18 +++++++++--------- web/lib/aur.inc | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/web/html/passreset.php b/web/html/passreset.php index 2c7801d..0ce6f7d 100644 --- a/web/html/passreset.php +++ b/web/html/passreset.php @@ -58,8 +58,8 @@ if (isset($_GET['resetkey'], $_POST['email'], $_POST['password'], $_POST['confir $resetkey = new_sid(); $dbh = db_connect(); $q = "UPDATE Users - SET ResetKey = '$resetkey' - WHERE ID = '$uid'"; + SET ResetKey = '" . $resetkey . "' + WHERE ID = " . $uid; db_query($q, $dbh); # Send email with confirmation link $body = __('A password reset request was submitted for the account '. diff --git a/web/html/pkgsubmit.php b/web/html/pkgsubmit.php index 5797626..3ef5823 100644 --- a/web/html/pkgsubmit.php +++ b/web/html/pkgsubmit.php @@ -320,7 +320,7 @@ if ($_COOKIE["AURSID"]): } # Update package data - $q = sprintf("UPDATE Packages SET ModifiedTS = UNIX_TIMESTAMP(), Name = '%s', Version = '%s-%s', License = '%s', Description = '%s', URL = '%s', OutOfDateTS = NULL, MaintainerUID = '%d' WHERE ID = %d", + $q = sprintf("UPDATE Packages SET ModifiedTS = UNIX_TIMESTAMP(), Name = '%s', Version = '%s-%s', License = '%s', Description = '%s', URL = '%s', OutOfDateTS = NULL, MaintainerUID = %d WHERE ID = %d", mysql_real_escape_string($new_pkgbuild['pkgname']), mysql_real_escape_string($new_pkgbuild['pkgver']), mysql_real_escape_string($new_pkgbuild['pkgrel']), diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc index 8e2ecb3..8ffa2f7 100644 --- a/web/lib/acctfuncs.inc +++ b/web/lib/acctfuncs.inc @@ -619,18 +619,18 @@ function try_login() { # last ($MAX_SESSIONS_PER_USER - 1). $q = "DELETE s.* FROM Sessions s "; $q.= "LEFT JOIN (SELECT SessionID FROM Sessions "; - $q.= "WHERE UsersId = '" . $userID . "' "; + $q.= "WHERE UsersId = " . $userID . " "; $q.= "ORDER BY LastUpdateTS DESC "; $q.= "LIMIT " . ($MAX_SESSIONS_PER_USER - 1) . ") q "; $q.= "ON s.SessionID = q.SessionID "; - $q.= "WHERE s.UsersId = '" . $userID . "' "; + $q.= "WHERE s.UsersId = " . $userID . " "; $q.= "AND q.SessionID IS NULL;"; db_query($q, $dbh); } $new_sid = new_sid(); $q = "INSERT INTO Sessions (UsersID, SessionID, LastUpdateTS)" - ." VALUES ( $userID, '" . $new_sid . "', UNIX_TIMESTAMP())"; + ." VALUES (" . $userID . ", '" . $new_sid . "', UNIX_TIMESTAMP())"; $result = db_query($q, $dbh); # Query will fail if $new_sid is not unique @@ -749,7 +749,7 @@ function valid_passwd( $userID, $passwd ) if ($salt) { # use salt $passwd_q = "SELECT ID FROM Users" . - " WHERE ID = '$userID' AND Passwd = '" . + " WHERE ID = " . $userID . " AND Passwd = '" . salted_hash($passwd, $salt) . "'"; $passwd_result = mysql_fetch_row(db_query($passwd_q, $dbh)); if ($passwd_result[0]) { @@ -758,14 +758,14 @@ function valid_passwd( $userID, $passwd ) } else { # check without salt $nosalt_q = "SELECT ID FROM Users". - " WHERE ID = '$userID'" . + " WHERE ID = " . $userID . " AND Passwd = '" . md5($passwd) . "'"; $nosalt_result = mysql_fetch_row(db_query($nosalt_q, $dbh)); if ($nosalt_result[0]) { # password correct, but salt it first if (!save_salt($userID, $passwd)) { trigger_error("Unable to salt user's password;" . - " ID $userID", E_USER_WARNING); + " ID " . $userID, E_USER_WARNING); return false; } @@ -782,7 +782,7 @@ function valid_passwd( $userID, $passwd ) function user_suspended( $id ) { $dbh = db_connect(); - $q = "SELECT Suspended FROM Users WHERE ID = '$id'"; + $q = "SELECT Suspended FROM Users WHERE ID = " . $id; $result = mysql_fetch_row(db_query($q, $dbh)); if ($result[0] == 1 ) { return true; @@ -796,7 +796,7 @@ function user_suspended( $id ) function user_delete( $id ) { $dbh = db_connect(); - $q = "DELETE FROM Users WHERE ID = '$id'"; + $q = "DELETE FROM Users WHERE ID = " . $id; $result = mysql_fetch_row(db_query($q, $dbh)); return; } @@ -808,7 +808,7 @@ function user_delete( $id ) function user_is_privileged( $id ) { $dbh = db_connect(); - $q = "SELECT AccountTypeID FROM Users WHERE ID = '$id'"; + $q = "SELECT AccountTypeID FROM Users WHERE ID = " . $id; $result = mysql_fetch_row(db_query($q, $dbh)); if( $result[0] > 1) { return $result[0]; diff --git a/web/lib/aur.inc b/web/lib/aur.inc index 744b31e..66ae1c2 100644 --- a/web/lib/aur.inc +++ b/web/lib/aur.inc @@ -478,7 +478,7 @@ function mkurl($append) { function get_salt($user_id) { $dbh = db_connect(); - $salt_q = "SELECT Salt FROM Users WHERE ID = '$user_id'"; + $salt_q = "SELECT Salt FROM Users WHERE ID = " . $user_id; $salt_result = mysql_fetch_row(db_query($salt_q, $dbh)); return $salt_result[0]; } @@ -488,8 +488,8 @@ function save_salt($user_id, $passwd) $dbh = db_connect(); $salt = generate_salt(); $hash = salted_hash($passwd, $salt); - $salting_q = "UPDATE Users SET Salt = '$salt'" . - ", Passwd = '$hash' WHERE ID = '$user_id'"; + $salting_q = "UPDATE Users SET Salt = '" . $salt . "', " . + "Passwd = '" . $hash . "' WHERE ID = " . $user_id; return db_query($salting_q, $dbh); } -- 1.7.5

elij

7:04 a.m.

New subject: [aur-dev] [PATCH 2/3] SQL: treat all UID/ID values as numbers, not strings

On Mon, Apr 25, 2011 at 9:23 PM, Dan McGee wrote:

...

Ensure we are not quoting these values in any of our SQL queries.

wewt!

...

Thanks-to: elij Signed-off-by: Dan McGee ---

Smoke tested: * Submitted a package update and it worked * Updated a user's password from non-salted to salted variety * Numerous login/logout cycles

web/html/passreset.php | 4 ++-- web/html/pkgsubmit.php | 2 +- web/lib/acctfuncs.inc | 18 +++++++++--------- web/lib/aur.inc | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/web/html/passreset.php b/web/html/passreset.php index 2c7801d..0ce6f7d 100644 --- a/web/html/passreset.php +++ b/web/html/passreset.php @@ -58,8 +58,8 @@ if (isset($_GET['resetkey'], $_POST['email'], $_POST['password'], $_POST['confir $resetkey = new_sid(); $dbh = db_connect(); $q = "UPDATE Users - SET ResetKey = '$resetkey' - WHERE ID = '$uid'"; + SET ResetKey = '" . $resetkey . "' + WHERE ID = " . $uid; db_query($q, $dbh); # Send email with confirmation link $body = __('A password reset request was submitted for the account '. diff --git a/web/html/pkgsubmit.php b/web/html/pkgsubmit.php index 5797626..3ef5823 100644 --- a/web/html/pkgsubmit.php +++ b/web/html/pkgsubmit.php @@ -320,7 +320,7 @@ if ($_COOKIE["AURSID"]): }

# Update package data - $q = sprintf("UPDATE Packages SET ModifiedTS = UNIX_TIMESTAMP(), Name = '%s', Version = '%s-%s', License = '%s', Description = '%s', URL = '%s', OutOfDateTS = NULL, MaintainerUID = '%d' WHERE ID = %d", + $q = sprintf("UPDATE Packages SET ModifiedTS = UNIX_TIMESTAMP(), Name = '%s', Version = '%s-%s', License = '%s', Description = '%s', URL = '%s', OutOfDateTS = NULL, MaintainerUID = %d WHERE ID = %d", mysql_real_escape_string($new_pkgbuild['pkgname']), mysql_real_escape_string($new_pkgbuild['pkgver']), mysql_real_escape_string($new_pkgbuild['pkgrel']), diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc index 8e2ecb3..8ffa2f7 100644 --- a/web/lib/acctfuncs.inc +++ b/web/lib/acctfuncs.inc @@ -619,18 +619,18 @@ function try_login() { # last ($MAX_SESSIONS_PER_USER - 1). $q = "DELETE s.* FROM Sessions s "; $q.= "LEFT JOIN (SELECT SessionID FROM Sessions "; - $q.= "WHERE UsersId = '" . $userID . "' "; + $q.= "WHERE UsersId = " . $userID . " "; $q.= "ORDER BY LastUpdateTS DESC "; $q.= "LIMIT " . ($MAX_SESSIONS_PER_USER - 1) . ") q "; $q.= "ON s.SessionID = q.SessionID "; - $q.= "WHERE s.UsersId = '" . $userID . "' "; + $q.= "WHERE s.UsersId = " . $userID . " "; $q.= "AND q.SessionID IS NULL;"; db_query($q, $dbh); }

$new_sid = new_sid(); $q = "INSERT INTO Sessions (UsersID, SessionID, LastUpdateTS)" - ." VALUES ( $userID, '" . $new_sid . "', UNIX_TIMESTAMP())"; + ." VALUES (" . $userID . ", '" . $new_sid . "', UNIX_TIMESTAMP())"; $result = db_query($q, $dbh);

# Query will fail if $new_sid is not unique @@ -749,7 +749,7 @@ function valid_passwd( $userID, $passwd ) if ($salt) { # use salt $passwd_q = "SELECT ID FROM Users" . - " WHERE ID = '$userID' AND Passwd = '" . + " WHERE ID = " . $userID . " AND Passwd = '" . salted_hash($passwd, $salt) . "'"; $passwd_result = mysql_fetch_row(db_query($passwd_q, $dbh)); if ($passwd_result[0]) { @@ -758,14 +758,14 @@ function valid_passwd( $userID, $passwd ) } else { # check without salt $nosalt_q = "SELECT ID FROM Users". - " WHERE ID = '$userID'" . + " WHERE ID = " . $userID . " AND Passwd = '" . md5($passwd) . "'"; $nosalt_result = mysql_fetch_row(db_query($nosalt_q, $dbh)); if ($nosalt_result[0]) { # password correct, but salt it first if (!save_salt($userID, $passwd)) { trigger_error("Unable to salt user's password;" . - " ID $userID", E_USER_WARNING); + " ID " . $userID, E_USER_WARNING); return false; }

@@ -782,7 +782,7 @@ function valid_passwd( $userID, $passwd ) function user_suspended( $id ) { $dbh = db_connect(); - $q = "SELECT Suspended FROM Users WHERE ID = '$id'"; + $q = "SELECT Suspended FROM Users WHERE ID = " . $id; $result = mysql_fetch_row(db_query($q, $dbh)); if ($result[0] == 1 ) { return true; @@ -796,7 +796,7 @@ function user_suspended( $id ) function user_delete( $id ) { $dbh = db_connect(); - $q = "DELETE FROM Users WHERE ID = '$id'"; + $q = "DELETE FROM Users WHERE ID = " . $id; $result = mysql_fetch_row(db_query($q, $dbh)); return; } @@ -808,7 +808,7 @@ function user_delete( $id ) function user_is_privileged( $id ) { $dbh = db_connect(); - $q = "SELECT AccountTypeID FROM Users WHERE ID = '$id'"; + $q = "SELECT AccountTypeID FROM Users WHERE ID = " . $id; $result = mysql_fetch_row(db_query($q, $dbh)); if( $result[0] > 1) { return $result[0]; diff --git a/web/lib/aur.inc b/web/lib/aur.inc index 744b31e..66ae1c2 100644 --- a/web/lib/aur.inc +++ b/web/lib/aur.inc @@ -478,7 +478,7 @@ function mkurl($append) { function get_salt($user_id) { $dbh = db_connect(); - $salt_q = "SELECT Salt FROM Users WHERE ID = '$user_id'"; + $salt_q = "SELECT Salt FROM Users WHERE ID = " . $user_id; $salt_result = mysql_fetch_row(db_query($salt_q, $dbh)); return $salt_result[0]; } @@ -488,8 +488,8 @@ function save_salt($user_id, $passwd) $dbh = db_connect(); $salt = generate_salt(); $hash = salted_hash($passwd, $salt); - $salting_q = "UPDATE Users SET Salt = '$salt'" . - ", Passwd = '$hash' WHERE ID = '$user_id'"; + $salting_q = "UPDATE Users SET Salt = '" . $salt . "', " . + "Passwd = '" . $hash . "' WHERE ID = " . $user_id; return db_query($salting_q, $dbh); }

-- 1.7.5

Lukas Fleischer

5:04 p.m.

New subject: [aur-dev] [PATCH 2/3] SQL: treat all UID/ID values as numbers, not strings

On Mon, Apr 25, 2011 at 11:23:01PM -0500, Dan McGee wrote:

...

Ensure we are not quoting these values in any of our SQL queries.

Thanks-to: elij Signed-off-by: Dan McGee ---

Smoke tested: * Submitted a package update and it worked * Updated a user's password from non-salted to salted variety * Numerous login/logout cycles

web/html/passreset.php | 4 ++-- web/html/pkgsubmit.php | 2 +- web/lib/acctfuncs.inc | 18 +++++++++--------- web/lib/aur.inc | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-)

Mhh. Basically looks good to me :) Did you check if all affected variables are correctly coerced into integers? If there's any unquoted parameter that is escaped using mysql_real_escape_string() instead of intval() (or something similar), we might become vulnerable to SQL injections, again. We should have used integer conversions in the first place, of course, but this patch will probably turn any mysql_real_escape_string() misuse into an exploitable injection vulnerability.

Dan McGee

5:44 p.m.

New subject: [aur-dev] [PATCH 2/3] SQL: treat all UID/ID values as numbers, not strings

On Tue, Apr 26, 2011 at 10:04 AM, Lukas Fleischer wrote:

...

On Mon, Apr 25, 2011 at 11:23:01PM -0500, Dan McGee wrote:

...
Ensure we are not quoting these values in any of our SQL queries.

Thanks-to: elij Signed-off-by: Dan McGee ---

Smoke tested: * Submitted a package update and it worked * Updated a user's password from non-salted to salted variety * Numerous login/logout cycles

web/html/passreset.php | 4 ++-- web/html/pkgsubmit.php | 2 +- web/lib/acctfuncs.inc | 18 +++++++++--------- web/lib/aur.inc | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-)

Mhh. Basically looks good to me :)

Did you check if all affected variables are correctly coerced into integers? If there's any unquoted parameter that is escaped using mysql_real_escape_string() instead of intval() (or something similar), we might become vulnerable to SQL injections, again.

We should have used integer conversions in the first place, of course, but this patch will probably turn any mysql_real_escape_string() misuse into an exploitable injection vulnerability.

I did not; you may want to take a quick peek- I know you've been more on the ball with fixing these than me, so you can probably spot the code paths real quick. I more just made sure things worked as before. -Dan

Lukas Fleischer

27 Apr 27 Apr

6:30 p.m.

New subject: [aur-dev] [PATCH 2/3] SQL: treat all UID/ID values as numbers, not strings

On Tue, Apr 26, 2011 at 10:44:33AM -0500, Dan McGee wrote:

...

On Tue, Apr 26, 2011 at 10:04 AM, Lukas Fleischer wrote:

...
On Mon, Apr 25, 2011 at 11:23:01PM -0500, Dan McGee wrote:

...
Ensure we are not quoting these values in any of our SQL queries.

Thanks-to: elij Signed-off-by: Dan McGee ---

Smoke tested: * Submitted a package update and it worked * Updated a user's password from non-salted to salted variety * Numerous login/logout cycles

web/html/passreset.php | 4 ++-- web/html/pkgsubmit.php | 2 +- web/lib/acctfuncs.inc | 18 +++++++++--------- web/lib/aur.inc | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-)

Mhh. Basically looks good to me :)

Did you check if all affected variables are correctly coerced into integers? If there's any unquoted parameter that is escaped using mysql_real_escape_string() instead of intval() (or something similar), we might become vulnerable to SQL injections, again.

We should have used integer conversions in the first place, of course, but this patch will probably turn any mysql_real_escape_string() misuse into an exploitable injection vulnerability.

I did not; you may want to take a quick peek- I know you've been more on the ball with fixing these than me, so you can probably spot the code paths real quick. I more just made sure things worked as before.

Skimming through the affected functions, it looks like there shouldn't be any security regression introduced by this. Pushed that (along with the other patches this series).

Dan McGee

26 Apr 26 Apr

6:23 a.m.

New subject: [aur-dev] [PATCH 3/3] stats: capitalize FROM in all SQL queries

Matches our normal code conventions. Signed-off-by: Dan McGee --- web/lib/stats.inc | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/web/lib/stats.inc b/web/lib/stats.inc index 75b2537..756fa27 100644 --- a/web/lib/stats.inc +++ b/web/lib/stats.inc @@ -80,25 +80,25 @@ function general_stats_table($dbh) $q = "SELECT count(*) FROM Packages WHERE MaintainerUID IS NULL"; $orphan_count = db_cache_value($q, $dbh, $apc_prefix . 'orphan_count'); - $q = "SELECT count(*) from Users"; + $q = "SELECT count(*) FROM Users"; $user_count = db_cache_value($q, $dbh, $apc_prefix . 'user_count'); - $q = "SELECT count(*) from Users,AccountTypes WHERE Users.AccountTypeID = AccountTypes.ID AND AccountTypes.AccountType = 'Trusted User'"; + $q = "SELECT count(*) FROM Users,AccountTypes WHERE Users.AccountTypeID = AccountTypes.ID AND AccountTypes.AccountType = 'Trusted User'"; $tu_count = db_cache_value($q, $dbh, $apc_prefix . 'tu_count'); $targstamp = intval(strtotime("-7 days")); $yearstamp = intval(strtotime("-1 year")); - $q = "SELECT count(*) from Packages WHERE Packages.ModifiedTS >= $targstamp AND Packages.ModifiedTS = Packages.SubmittedTS"; + $q = "SELECT count(*) FROM Packages WHERE Packages.ModifiedTS >= $targstamp AND Packages.ModifiedTS = Packages.SubmittedTS"; $add_count = db_cache_value($q, $dbh, $apc_prefix . 'add_count'); - $q = "SELECT count(*) from Packages WHERE Packages.ModifiedTS >= $targstamp AND Packages.ModifiedTS != Packages.SubmittedTS"; + $q = "SELECT count(*) FROM Packages WHERE Packages.ModifiedTS >= $targstamp AND Packages.ModifiedTS != Packages.SubmittedTS"; $update_count = db_cache_value($q, $dbh, $apc_prefix . 'update_count'); - $q = "SELECT count(*) from Packages WHERE Packages.ModifiedTS >= $yearstamp AND Packages.ModifiedTS != Packages.SubmittedTS"; + $q = "SELECT count(*) FROM Packages WHERE Packages.ModifiedTS >= $yearstamp AND Packages.ModifiedTS != Packages.SubmittedTS"; $update_year_count = db_cache_value($q, $dbh, $apc_prefix . 'update_year_count'); - $q = "SELECT count(*) from Packages WHERE Packages.ModifiedTS = Packages.SubmittedTS"; + $q = "SELECT count(*) FROM Packages WHERE Packages.ModifiedTS = Packages.SubmittedTS"; $never_update_count = db_cache_value($q, $dbh, $apc_prefix . 'never_update_count'); include('stats/general_stats_table.php'); -- 1.7.5

4754

Age (days ago)

4755

Last active (days ago)

List overview

Download

12 comments

4 participants

participants (4)

Dan McGee
Dan McGee
elij
Lukas Fleischer