0

I am new to using mysqli_multi_query and haven't spent loads of time in php. My sql succeeds when I apply it directly in my db client (phpMyAdmin), but it does not execute from my php code. The return from this function is intented to be a 2-item array populated with the results of the 2 SELECT statements, but it always comes back empty. (Note: the connection succeeds; $con is defined elsewhere.)

function claimItem($key){
    //remove expiry, decrement quantity available, send Kate & Sasha an email.
    global $con;
    $sql = "SELECT A.name, B.name, C.quantity, B.id FROM invitees A, items B, invitees_items C WHERE C.activation_key = '$key' AND C.items_id = B.id AND C.invitees_id = A.id; SELECT @qnt := quantity, @itm := items_id FROM invitees_items WHERE `activation_key` = '$key'; UPDATE items SET num_available = GREATEST(num_available - @qnt, 0) WHERE id = @itm; UPDATE invitees_items SET expiry = null, activation_key = null WHERE activation_key = '$key';";

    $multiArr = array(array(), array());

    if (mysqli_multi_query($con,$sql)){
      $qcount = 0;
      do
        {
        // Store first result set
        $result=mysqli_store_result($con);
          // Fetch one and one row
          while ($row=mysqli_fetch_row($result))
            {
                array_push($multiArr[$qcount], $row);
            }
          // Free result set
          mysqli_free_result($result);

          $qcount++;
        }
      while (mysqli_next_result($con));
    }
    else {
        return "failure";
    }

    mysqli_close($con); 
    return $multiArr;
}
Sasha
  • 3
  • 2
  • 2
    Don't use `mysqli_multi_query`, it's extremely dangerous from a security perspective. – tadman Jun 04 '18 at 15:55
  • 1
    **WARNING**: When using `mysqli` you should be using [parameterized queries](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and [`bind_param`](http://php.net/manual/en/mysqli-stmt.bind-param.php) to add user data to your query. **DO NOT** use string interpolation or concatenation to accomplish this because you have created a severe [SQL injection bug](http://bobby-tables.com/). **NEVER** put `$_POST`, `$_GET` or **any** user data directly into a query, it can be very harmful if someone seeks to exploit your mistake. – tadman Jun 04 '18 at 15:55
  • Note: The object-oriented interface to `mysqli` is significantly less verbose, making code easier to read and audit, and is not easily confused with the obsolete `mysql_query` interface. Before you get too invested in the procedural style it’s worth switching over. Example: `$db = new mysqli(…)` and `$db->prepare("…")` The procedural interface is an artifact from the PHP 4 era when `mysqli` API was introduced and should not be used in new code. – tadman Jun 04 '18 at 15:55
  • Thanks, tadman, I appreciate the advice. I'll take these suggestions on to improve the security after I get this working, but for now I'm still stuck wondering why the sql doesn't execute and the array doesn't populate. – Sasha Jun 04 '18 at 18:50
  • (fwiw I'm still at the dummy-data / internal-testing stage so there's no security risk as of now) – Sasha Jun 04 '18 at 18:54
  • Security issues are something you should address *now*, not later, as later you'll forget and if you forget [bad things happen](https://codecurmudgeon.com/wp/sql-injection-hall-of-shame/). It's easier to do it right the first time, way harder to fix it after the fact. – tadman Jun 04 '18 at 19:46

1 Answers1

0

There are some issues with variable names.

There's a variable $multiArr that is initialized, but doesn't get referenced until the end, where it gets returned. This explains why an empty array is being returned. (There's a different variable, named $multiArray that gets referenced in the body of the function).

There's also a variable $qcount that's initialized and referenced. But that variable doesn't get incremented. Instead, a different variable, named $count gets incremented.

I'd recommend squaring that away, before declaring there's a problem with mysqli_multi_query.

The code appears to use a pattern known to be vulnerable to SQL Injection. And using mysqli_multi_query is gaping wide open to nefarious exploits.

spencer7593
  • 106,611
  • 15
  • 112
  • 140
  • Thanks, Spencer, good eye — fixed the typos and still see no execution of the sql. I'll take on tadman's suggestions to improve the security after I get this working, but for now I'm still stuck wondering why the sql doesn't execute and the array doesn't populate. – Sasha Jun 04 '18 at 18:50