Thu Jun 10, 2021 1:08 pm

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Thu Jun 10, 2021 1:08 pm linkquote
Hi Jess. I thought that your system believed I was trying SQL injection because my search term used "replace" but I changed that to "change" as in "change brake pad" and still got an error like this one.



Thu Jun 10, 2021 1:53 pm

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Thu Jun 10, 2021 1:53 pm linkquote
ScooterWoman9988 wrote:
Hi Jess. I thought that your system believed I was trying SQL injection because my search term used "replace" but I changed that to "change" as in "change brake pad" and still got an error like this one.
Sorry about that. I will investigate.
Thu Jun 10, 2021 1:55 pm

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Thu Jun 10, 2021 1:55 pm linkquote
Were there any other parameters you used? Posts vs Threads? I tried with the same search string and it seemed to work, but I could very well be missing something.
Thu Jun 10, 2021 2:08 pm

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Thu Jun 10, 2021 2:08 pm linkquote
Nope. I just put in "replace brake pad" as the search term and hit enter. Got the error and changed "replace brake pad" to "change brake pad" and tried again. Maybe it has resolved itself.

EDIT: Yup. Seems to be working now. I have no idea what happened.
Thu Jun 10, 2021 8:46 pm

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Thu Jun 10, 2021 8:46 pm linkquote
ScooterWoman9988 wrote:
EDIT: Yup. Seems to be working now. I have no idea what happened.
My suspicion is that there was a record stuck in the search results table. I'm not totally certain why -- the search system is kind of a beast and I admit that I don't fully understand it in its entirety. If it happens again, let me know. And again, I appreciate the problem report.
Fri Jun 11, 2021 5:36 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 5:36 am linkquote
That sounds about right. I'll let you know if I see it again.
Fri Jun 11, 2021 5:53 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 5:53 am linkquote
The mystery deepens: I went and looked at the code that threw this error. The duplicate key that it's complaining about is generated -- immediately before the SQL operation -- by generating a random number. And it does appear to be properly seeded.

So I am more puzzled than ever as to how that could have happened.
Fri Jun 11, 2021 6:08 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 6:08 am linkquote
It's not generating a GUID? That would be far less likely to ever generate a dupe than a random number generator.
Fri Jun 11, 2021 6:12 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 6:12 am linkquote
ScooterWoman9988 wrote:
It's not generating a GUID? That would be far less likely to ever generate a dupe than a random number generator.
Oh, agreed completely. But this code dates to 2002, so...

Might need to make some mods here.

Last edited by jess on Fri Jun 11, 2021 6:12 am; edited 1 time in total
Fri Jun 11, 2021 6:12 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 6:12 am linkquote
I'm surprised it's not using an auto incrementing primary key field if it wants to use numbers for the primary key. No chance of duplication then.

Is there any way to change it to auto incrementing and remove the primary key from the INSERT command?
Fri Jun 11, 2021 6:15 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 6:15 am linkquote
ScooterWoman9988 wrote:
I'm surprised it's not using an auto incrementing primary key field if it wants to use numbers for the primary key. No chance of duplication then.
The original software was designed to run on a variety of database types, with a translation layer in between. And a number of those DB systems weren't very well developed at the time, so it looks like there was a lot of coding to the lowest common denominator. Auto-increment wasn't used much, if at all.

These days, of course it makes sense to use those capabilities, and they are widely available with virtually every database.
Fri Jun 11, 2021 6:25 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 6:25 am linkquote
Okay, digging a little deeper: it appears the strategy goes something like this:

1. Generate pseudorandom number, seeded with microtime()
2. If there's already a set of search results associated with the session_id (i.e. that user's session cookie) then update the database with the new search results and set the search_id to the new random number
3. If there ISN'T a set of search results associated with the session, insert using the search_id field (which must be unique)

So that explains why auto-increment isn't used. While search_id must be unique, it is secondary to the session_id.

My question is: why have search_id at all? Probably because session_id historically wasn't very reliable at the dawn of web apps. But still. This seems like a bad strategy.
Fri Jun 11, 2021 6:28 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 6:28 am linkquote
Got it. It's not a big problem, anyway. It might not happen again for years.
Fri Jun 11, 2021 6:30 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 6:30 am linkquote
ScooterWoman9988 wrote:
Got it. It's not a big problem, anyway. It might not happen again for years.
Yeah, but still. This feels off to me -- I think I'm still missing some piece of the puzzle.
Fri Jun 11, 2021 6:36 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 6:36 am linkquote
I don't think you're missing anything. If so wouldn't it have shown up a long time ago or happen on a regular basis? Seems to me the random number generator simply generated a couple of numbers that had already been used.
Fri Jun 11, 2021 6:42 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 6:42 am linkquote
ScooterWoman9988 wrote:
I don't think you're missing anything. If so wouldn't it have shown up a long time ago or happen on a regular basis? Seems to me the random number generator simply generated a couple of numbers that had already been used.
That's what feels off. The search results are pruned automatically. At any given moment, there are only maybe 10 - 20 sets of search results in the database. With a sufficiently large random number, the chances of a collision are exceedingly low -- so low as to be highly unlikely. And you got it twice in a row.

That feels like a hole in the logic.
Fri Jun 11, 2021 6:50 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 6:50 am linkquote
So you're saying the search results are held in a temp table of some sort? I thought they were INSERTed into a permanent table that has been holding all the search results since inception of the site.

OK, then, yeah. There's something off with the logic but given that it can't easily be duplicated is it worth taking a few years off your life trying to track it down? I wouldn't bother and would just chalk it up to one of those things that happens once every ten years or so.
Fri Jun 11, 2021 6:55 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 6:55 am linkquote
This might have something to do with it. This is the test to see if the update worked:
if ( !($result = $db->sql_query($sql)) || !$db->sql_affectedrows() )
The first clause is the standard form used throughout this forum software: execute the SQL command and check the result.

The second clause is much less common -- and I'm not sure why the original author felt it necessary to second-guess the first clause. It seems to me, since this is structured as an OR, that if sql_affectedrows() returns 0 (for whatever reason), then the overall if statement would be true and the code it encloses (which is the INSERT statement, btw) would execute.

That's my theory.
Fri Jun 11, 2021 6:59 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 6:59 am linkquote
ScooterWoman9988 wrote:
So you're saying the search results are held in a temp table of some sort? I thought they were INSERTed into a permanent table that has been holding all the search results since inception of the site.
Nope. Just a handful of them at any given moment. Their lifespan is defined by the session time, which I think is 30 minutes.
Fri Jun 11, 2021 7:43 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 7:43 am linkquote
jess wrote:
This might have something to do with it. This is the test to see if the update worked:
if ( !($result = $db->sql_query($sql)) || !$db->sql_affectedrows() )
The first clause is the standard form used throughout this forum software: execute the SQL command and check the result.

The second clause is much less common -- and I'm not sure why the original author felt it necessary to second-guess the first clause. It seems to me, since this is structured as an OR, that if sql_affectedrows() returns 0 (for whatever reason), then the overall if statement would be true and the code it encloses (which is the INSERT statement, btw) would execute.

That's my theory.
What comes after the if? That codes looks to me like if SQL command fails (in this case INSERT) OR affectedrows is false THEN (do something). I'm wondering what the do something is. Throw an error?
Fri Jun 11, 2021 7:55 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 7:55 am linkquote
ScooterWoman9988 wrote:
What comes after the if? That codes looks to me like if SQL command fails (in this case INSERT) OR affectedrows is false THEN (do something). I'm wondering what the do something is. Throw an error?
Here's the whole sequence:
mt_srand ((double) microtime() * 1000000);
$search_id = mt_rand();

$sql = "UPDATE " . SEARCH_TABLE . "
   SET search_id = $search_id, search_time = $current_time, search_array = '" . str_replace("\'", "''", $result_array) . "'
   WHERE session_id = '" . $userdata['session_id'] . "'";
if ( !($result = $db->sql_query($sql)) || !$db->sql_affectedrows() )
{
   $sql = "INSERT INTO " . SEARCH_TABLE . " (search_id, session_id, search_time, search_array)
   VALUES($search_id, '" . $userdata['session_id'] . "', $current_time, '" . str_replace("\'", "''", $result_array) . "')";
   if ( !($result = $db->sql_query($sql)) )
   {
      message_die(GENERAL_ERROR, 'Could not insert search results', '', __LINE__, __FILE__, $sql);
   }
}
I'm thinking that the first clause (the sql_query() that performs the actual UPDATE SQL statement) succeeds, but (for reasons still unknown) sql_affectedrows() returns 0, causing the INSERT to be executed with the same key.

That message_die() function call is what produced the error message you saw.
Fri Jun 11, 2021 8:17 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 8:17 am linkquote
Agreed. Seems odd though that any database engine would return an erroneous result on affected rows.
Fri Jun 11, 2021 8:25 am

Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
 
Petty Tyrant
GTS250 GTS300 MP3 500
Joined: 11 Oct 2005
Posts: 31026
Location: Bay Area, California
Fri Jun 11, 2021 8:25 am linkquote
ScooterWoman9988 wrote:
Seems odd though that any database engine would return an erroneous result on affected rows.
Agreed. Which is the only reason I haven't already refactored this code this morning. I am puzzled by how the conditions might actually present themselves.
Fri Jun 11, 2021 9:24 am

Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
 
Hooked
Genuine Buddy 125, Piaggio BV 250
Joined: 29 Apr 2021
Posts: 139
Location: NE CT
Fri Jun 11, 2021 9:24 am linkquote
Well, it does seem like it's returning a wrong result, either from the update or the rows affected. Weird.
  DoubleGood Vespa Scarves  

All Content Copyright 2005-2021 by Modern Vespa. All Rights Reserved.

Modern Vespa is a participant in the Amazon Services LLC Associates Program, an affiliate advertising program designed to provide a means for sites to earn advertising fees by advertising and linking to amazon.com.

Shop on Amazon Smile with Modern Vespa
[ Time: 0.1606s ][ Queries: 49 (0.1267s) ][ Debug on ][ 156 ][ Thing Two ]