-1
function generate_session_id( &$db )
{
    $user_sess_id = md5( uniqid( mt_rand(), true );

    try
    {
        $stmt = $db->prepare("SELECT COUNT(*) AS session_exists FROM sessions WHERE session_id = :session_id");
        $stmt->bindParam(':session_id', $user_sess_id);
        $stmt->execute();
        $result = $stmt->fetch( PDO::FETCH_ASSOC );

        if( $result['session_exists'] == 1 )
        {
            // Recursion !
            generate_session_id( $db );
        }      
        else
        {
            return $user_sess_id;
        }
    }
    catch( PDOException $e )
    {
        die( "generate_session_id(): " . $e->getMessage() );
    }
}

Is this function safe to use or are there any flaws in it? Its only purpose is to generate unique ID for each session.

TheMagician
  • 1,846
  • 7
  • 20
  • 26

2 Answers2

2

You're not returning the value of the recursive function, so in case the function is invoked recursively, you'd get no value back. You need to do:

return generate_session_id( $db );

You don't need recursion at all here though. Just do a normal loop:

do {
    // generate id
    $id_exists = // look if id exists
} while ($id_exists);

Also, do you really need to generate the ID yourself? Are you using some session handling that requires this?

deceze
  • 510,633
  • 85
  • 743
  • 889
1

Looks pretty safe. But isn't the point of uniqid that it provides a, er, unique id? And why are you doing an MD5 hash on it? That introduces a whole range of problems...

Jeff Ober
  • 4,967
  • 20
  • 15
  • php.net/uniqid recommends doing it this way. I'm gonna put the id to database and cookie so I can check if someone tries to maliciously use stolen cookie to hijack someone's account. – TheMagician Nov 11 '09 at 03:42
  • Look at the first comment on the `uniqid` thread: http://php.net/manual/en/function.uniqid.php#91126 :) IMHO it's only okay if you need a string in the format of an md5 hash (character or length limit). – deceze Nov 11 '09 at 03:46