1

This is a 12 year old script written in php 5.3 or something which I recently upgraded to 5.6.

It interfaces with paypal parallel payments (grandfathered in) and the bug (not sure how it survived this long) is this:

  1. User A goes to the payment page
  2. $order_id is generated: mysql_query("select max(order_id)+ 1 as id from orders");
  3. $order_id is inserted into a couple of other tables
  4. User A is redirected to Paypal

Meanwhile

  1. User B goes to the payment page.
  2. $order_id is the same because orders table hasn't been updated yet

So now whichever users order processes through paypal SECOND and returns success to the payment script, the INSERT statement will fail because $order_id has already been inserted into the table.

We are (supposedly) replacing the whole system in a month or two and need a quick fix.

My first thought was:

//source: https://stackoverflow.com/a/3146986/2223106
define('MYSQL_CODE_DUPLICATE_KEY', 1062);
mysql_query($sql);
if (mysql_errno() == MYSQL_CODE_DUPLICATE_KEY) {
    $Invoice_id++;
    $sql = "INSERT etc

But now I realize maybe I can define a global $order_id and check it against mysql_query("select max(order_id)+ 1 as id from orders");, then if it matches the current next-id increment it by one.

Does this seems like a reasonable approach:

$excqry=mysql_query("select max(order_id)+ 1 as id from orders ") or die(mysql_error());

if(mysql_num_rows($excqry) > 0) {

  $row1=mysql_fetch_array($excqry);

  $Invoice_id = $row1['id'];

  if (isset($GLOBALS['order_id']) && ($GLOBALS['order_id'] <= $Invoice_id)){
    $GLOBALS['order_id'] = $Invoice_id + 1;
    $Invoice_id++;
  }

  // Continue with our MySQL statements

Obviously this doesn't deal with User C potentially coming along at the same time, but the site only has a few dozen users a week.

aknosis
  • 3,602
  • 20
  • 33
MikeiLL
  • 6,282
  • 5
  • 37
  • 68
  • 2
    Seems like there is already a "no duplicate" constraint on the column, can't you just simply make it a primary key(which it should be) and make it auto-incremental ? Globals are just bad idea and won't solve your problem – Himan Oct 04 '17 at 03:07
  • 2
    Why not to use `AUTO_INCREMENT` for `order_id` and store your order in DB before assigning `$order_id` to other tables? Just store your order in DB and use something like `mysql_insert_id` or its analogues? – Rulisp Oct 04 '17 at 03:40
  • Consider upgrading from `mysql` to `mysqli`, `mysql` is deprecated in 5.x and removed in 7.x. – aknosis Oct 04 '17 at 04:00
  • @Aknosis i know that at least on this page it would be fairly elementary to upgrade to mysqli and will if the codebase needs to live on much longer. But that wouldn't address this problem, would it? – MikeiLL Oct 04 '17 at 04:15
  • @Rulisp you mean to insert a "blank" row immediately, returning the id with `mysql_insert_id` for insertion into other tables, then updating whichever row matches the variable holding the `mysql_insert_id` value once the order processes, or potentially DELETEing it if the order fails? – MikeiLL Oct 04 '17 at 04:19
  • @Himan it is an `auto_increment` column and PRIMARY KEY. I think what @Rulisp is suggesting–reiterated in my previous comment–makes sense. Do you agree? – MikeiLL Oct 04 '17 at 04:23
  • 1
    @MikeiLL that's right. Just define `order_id` as a `PRIMARY KEY`, add to it `AUTO_INCREMENT`. Then use `mysql_insert_id` to retrieve `order_id` and that's all – Rulisp Oct 04 '17 at 04:29
  • @MikeiLL , so, did it helped you? May post it as an answer?:) – Rulisp Oct 04 '17 at 05:17
  • @MikeiLL It won't solve your issue in this question. It is just a word of advice. Supposed you need to upgrade php for security reasons. It would be painful to update then. – aknosis Oct 04 '17 at 15:15
  • @Rulisp yes please. Or if you don't feel like it let me know and I'll post my own answer. – MikeiLL Oct 04 '17 at 19:16
  • @Aknosis if the codebase is going to persist will certainly upgrade and I'm sure it will be painful. But I think we are moving on. – MikeiLL Oct 04 '17 at 19:17

2 Answers2

2

$MikeiLL I also thinks its better to keep order_id as primary so it will be increment by itself instead of tracking of it, GLOBAL isn't a good Idead

shashi
  • 281
  • 1
  • 13
1

So, to solve your problem, you should make following steps

  1. Define order_id as a PRIMARY KEY
  2. Add to it AUTO_INCREMENT.
  3. Then, in your controller, you store your order immediately after all checks and then use mysql_insert_id to retrieve order_id
  4. Now, you can insert $order_id anywhere you need
Rulisp
  • 1,586
  • 1
  • 18
  • 30