3
\$\begingroup\$

I'm posting my PHP code for sessions using MySQL. Please tell me if there are any errors, if it can be optimized, or if security can be enhanced.

  • Host: localhost
  • User: root
  • Password: ""
  • Database: pro

Table structure:

SessionID [pk] - Data - DateTouched

<?php
 function open($sess_id, $sess_name) {
  return true;
 }

 function close() {
  return true;
 }

 function read($sess_id) {
  $con = mysqli_connect("localhost", "root", "","pro");
  $stmt = mysqli_prepare($con,"SELECT Data FROM sessions WHERE SessionID = ?");
  mysqli_stmt_bind_param($stmt,"s",$sess_id);
  mysqli_stmt_execute($stmt);
  mysqli_stmt_bind_result($stmt,$data);
  mysqli_stmt_fetch($stmt);
  mysqli_stmt_close($stmt);
  $CurrentTime = date('Y-m-d H:i:s');
  if (!isset($data))
  {
   $stmt = mysqli_prepare($con,"INSERT INTO sessions (SessionID, DateTouched) VALUES (?,?)");
   mysqli_stmt_bind_param($stmt,"ss",$sess_id,$CurrentTime);
   mysqli_stmt_execute($stmt);
   mysqli_stmt_close($stmt);
   return false;
  }
  else
  {
   $stmt = mysqli_prepare($con,"UPDATE sessions SET DateTouched = ? WHERE SessionID = ?");
   mysqli_stmt_bind_param($stmt,"ss",$CurrentTime,$sess_id);
   mysqli_stmt_execute($stmt);
   mysqli_stmt_close($stmt);
   return $data;
  }
 }

  function write($sess_id, $data) {
   $con = mysqli_connect("localhost", "root", "","pro");
   $CurrentTime = date('Y-m-d H:i:s');
   $stmt = mysqli_prepare($con,"UPDATE sessions SET Data= ?,DateTouched=? WHERE SessionID=?");
   mysqli_stmt_bind_param($stmt,"sss",$data,$CurrentTime,$sess_id);
   mysqli_stmt_execute($stmt);
   return true;
  }

   function destroy($sess_id) {
   $con = mysqli_connect("localhost", "root", "","pro");
   $stmt = mysqli_prepare($con,"DELETE FROM sessions WHERE SessionID = ?");
   mysqli_stmt_bind_param($stmt,"s",$sess_id);
   mysqli_stmt_execute($stmt);
   return true;
  }

   function gc($sess_maxlifetime) 
  {
   $con = mysqli_connect("localhost", "root", "","pro");
   $stmt = mysqli_prepare($con,"Delete from sessions where TIMESTAMPDIFF (Second ,DateTouched,now())>=?");
   mysqli_stmt_bind_param($stmt,"s",$sess_maxlifetime);
   mysqli_stmt_execute($stmt);
   return true;
  }

  session_set_save_handler("open", "close", "read", "write", "destroy", "gc");
  session_name('Session');
  session_start();
 ?>
\$\endgroup\$
2
\$\begingroup\$

Your open and close functions don't do anything. You could use them to connect & disconnect from the database. That way, an error in the connection won't be a read or write error, but will be an error in open. That may later help diagnosing such errors. It also means less duplicated code and it may mean fewer connections to the database (which might speed up things a bit).

\$\endgroup\$
  • \$\begingroup\$ good idea ronald, anything else you will suggest ? \$\endgroup\$ – Sourav May 8 '11 at 17:59
  • 1
    \$\begingroup\$ @Sourav Well, personally I'd put all of it in a class so I could carry the connection around in an object... \$\endgroup\$ – rlc May 9 '11 at 14:21
  • \$\begingroup\$ anything for security or increasing performance ? \$\endgroup\$ – Sourav May 9 '11 at 14:23
1
\$\begingroup\$

Especially in ajaxy environments, a lot of different request can be opened to your server. In their current implementation there is no check or lock for that, which means that setting a session variable somewhere can be undone by a session save in another request.

  1. Request A open
  2. Request B open
  3. A changes data
  4. A exist & saves
  5. B exists & saves stale data

I'd recommand either doing an SELECT ..... FOR UPDATE or checking last-opened-time with last-changed-time in the database.

\$\endgroup\$
  • \$\begingroup\$ plz explain a little bit more ! \$\endgroup\$ – Sourav Jun 8 '11 at 2:30
  • \$\begingroup\$ What do you need more explanation of? You can see PHP's file-based session solution in action by creating a file with <?php session_start();sleep(60);?>, and while that is loading, try to open any other page with a session_start() in it: it won't load any further beyond the blocking session_start() until the other request is finished. Doing a SELECT .... FOR UPDATE is pretty much the same thing, only it locks a row in the database (i.e. it cannot be selected until the connection having the lock either releases it or exits). \$\endgroup\$ – Wrikken Jun 8 '11 at 18:04
  • \$\begingroup\$ If you do so, make the gc a cronjob rather then in the code of a request itself: the gc handler will also block until the lock on that row is released. To avoid something like that, you could store a token (not a timestamp, microseconds do count) in the row in the database and only allow writing if the token has stayed the same, but that would mean 2 simultaneous requests (that are not blocked in this manner) could want to write different changes to the session in the database. You'll have to think of something clever in your application to handle that if you support concurrent requests. \$\endgroup\$ – Wrikken Jun 8 '11 at 18:09
  • \$\begingroup\$ but InnoDB only the row gets locked not the table (as in MyISAM), and as a user should only have a single session then i dont think it's problem. \$\endgroup\$ – Sourav Jun 9 '11 at 2:55
  • \$\begingroup\$ Yes, but the gc needs access to all rows afaik... \$\endgroup\$ – Wrikken Jun 9 '11 at 8:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.