I've had this problem with multiple scripts. Basically, it happens when the user submits the form or url many times by refreshing the page. Example: $user = mysql query select field from table where id = 1 if($user[field] >= 1){ // script mysql query update table set field=field-1 } // this is just an example to get the point across. Obviously it wouldn't work like that. Now what happens when the user keeps hammering the refresh key or submitting the info in some other way, the field 'field' ends up with a negative amount such as -3 (filed type is int). If I make it unsigned, then it explodes to the maximum possible value. There must surely be something that I am not aware of, because it's a major problem. How is it even possible that it gets past the "if($user[field] >= 1){ " if field is less than 0? Obviously client side delays wouldn't work because they can easily be bypassed.
Post your code. field - 1 is going to minus 1 from the current value of field every time you refresh the page.
Sorry, I forgot to edit it. It was supposed to be if($user[field] >= 1){ It does not happen if I submit the info like a normally would. Anyway, here is a code that it actually happens with. It's a lot heavier in reality. I deleted most of the unnecessary queries. But it shouldn't matter. <? function buymoves($usern, $type, $amount, $cash, $owner){ global $table, $time, $senderip; // pulluser $dauser = mysql_fetch_array(mysql_query("SELECT * FROM $table[user] WHERE username='$usern';")); $daowner = mysql_fetch_array(mysql_query("SELECT * FROM $table[user] WHERE username='$owner';")); $game = mysql_fetch_array(mysql_query("SELECT round FROM $table[game] WHERE active=1;")); //echo $dauser[username]; if($dauser && $type > 0 && $amount > 0 && $cash > 0){ // just to make sure everything is set if($daowner['cash'] < $cash){ // make sure that he has enough money $error = "Not enough cash."; }elseif($daowner['cash'] >= $cash){ // has enough mysql_query("UPDATE users SET cash=cash-$cash WHERE username='$daowner[username]'"); /////////////////// moves //////////////////// /////////////////// moves //////////////////// /////////////////// moves //////////////////// if($type == 1 && ($amount == 50 || $amount == 60 || $amount == 70)){ mysql_query("UPDATE users SET bought_moves=bought_moves+$amount WHERE username='$dauser[username]';"); } // QUERIES THAT ARE RAN EITHER WAY (UNIMPORTANT AND DO NOT INVOLVE THE FIELD 'CASH', just make the code heavier $error = "All done."; } } return $error; } ?> PHP: How is it possible that the user can bypass this: if($daowner['cash'] < $cash){ // make sure that he has enough money even if $cash is 0 or negative?
First what does $daowner['cash'] = ? and what does $cash = ? (without the below changes) then.... Try adding the following before the if statements: $cash = (int) $cash; $daowner['cash'] = (int) $daowner['cash']; PHP:
I believe I fixed all of the issues in your function, or at least wrote it to check for typecasts before doing queries and writing a bit cleaner. Hope it works for you. <?php function buymoves($username, $type = 0, $amount = 0, $cash = 0, $owner) { // Set default. $error = 'No Error Set.'; // Set the global variables. global $table, $time, $senderip; // Fetch the user data. $user_data = mysql_fetch_array(mysql_query(sprintf("SELECT * FROM {$table['user']} WHERE `username` = '%s' LIMIT 0, 1;", $username))); // Fetch the owner data. $owner_data = mysql_fetch_array(mysql_query(sprintf("SELECT * FROM {$table['user']} WHERE `username` = '%s' LIMIT 0, 1;", $owner))); // Fetch the game data. $game_data = mysql_fetch_array(mysql_query("SELECT `round` FROM {$table['game']} WHERE `active` = 1;")); //echo $dauser[username]; // If USER is set and not empty, TYPE greater than 0, AMOUNT greater than 0, and CASH greater than 0.. if(!empty($user_data) && $type > 0 && $amount > 0 && $cash > 0) { // If the user has enough cash.. if($owner_data['cash'] >= $cash) { // Set the USER cash for the new purchase. mysql_query(sprintf("UPDATE {$table['user']} SET `cash` = `cash` - %d WHERE `username` = '%s';", $cash, $owner_data['username'])); /////////////////// moves //////////////////// /////////////////// moves //////////////////// /////////////////// moves //////////////////// // If TYPE is 1 AND AMOUNT is either 50, 60, 70.. if($type == 1 && ($amount == 50 OR $amount == 60 OR $amount == 70)) { // Set USER bought_moves to the amount bought. mysql_query(sprintf("UPDATE {$table['user']} SET `bought_moves` = `bought_moves` + %d WHERE username = '%s';", $amount, $user_data['username'])); } // QUERIES THAT ARE RAN EITHER WAY (UNIMPORTANT AND DO NOT INVOLVE THE FIELD 'CASH', just make the code heavier // No errors. $error = "All done."; } else { // The USER didn't have enough cash. $error = "Not enough cash."; } } return $error; } ?> PHP:
without going too deep into your code and taking into account what you said about when the error happens i can say the following looks like you have multithreading problem by default mysql does not lock your tables when you perform a query, but in the case with your code it can be dangerous especially on a slow or overcrawded server try locking the table just before the query where you decrease the value of $cash and then unlock it right after the query. I didn't go too deep in your code, so if it doesn't work - try locking the table at the begining of the function and then unlocking at the end. The second way (locking during the whole function's runtime) is a bit too harsh way and you should be more intelligent and find the right place to lock it, however if you don't wanna bother with it too much - locking the table during the runtime of the function should definitely help
$daowner['cash'] is int and any number bigger than 0. When I managed to reproduce the error, I set it to 4. $cash is also a fixed number. Thanks for modifying my function. It's full of good tips. I'm always trying to code as clean as possible. My comments and questions: Can changes like that make a difference? Performance wise sure, but I'm not sure if it will take care of my problem. Anyway, I've never used sprintf before. Should I be using it in every query as you showed? I'm afraid that it will get confusing when more variables come into play. I've been wondering whether to use apostrophes inside queries around numeric variables. I think it's safer to use them because when something happens with the variable and it does not get set or is not even numeric, things can get pretty ugly. I'm also wondering why you changed the || for OR. I thought OR and AND were used for sql queries only, even though they work in php too. The apostrophe (`) you used around field names - adding this to every query will definitely take a while. Is it worth it? I'm not familiar with locking the table. I didn't even know that was possible. For a site that has hundreds of queries ran every second, would locking tables be problematic? Since there are many users using the same table at the same time. Basically, the script I have this issue with is quite heavy and it's not unusual for mysql to utilize 99% of the memory. This is probably where the problem lies, or a contributing factor. Still I feel a little betrayed by php. I have had fields explode to their maximum value. Actually, I forgot to add that I managed to reproduce the error on my local server as well, by refreshing the page many times as fast as possible. It's like the queries are ran too fast and it starts skipping the "checkpoints".
Most of it is preference. I like using sprintf because it allows you to typecast without having to do any major filtering/sanitizing with integers. The ` key isn't the same as the apostrophe ( ' ) key (for clarification). I believe it's a grave accent or something along those lines. You also don't need apostrophes around integers, but I don't believe it makes a difference. The ` key around the tables is just a habit that I have. It helps me read queries faster for when I go back to do revisions or refactors. Then there is the AND and OR in PHP. They are all right, but I follow the preference of the CodeIgniter code rules.. thing. Under this, you use && and OR to make compounded queries. Comments for your persisting issue.. You could try a small timeout and see if it does anything, but it shouldn't. Try finding a log of the queries submitted in MySQL.