WTF!!! Bug in the rcube_db.inc -> insert_id for postgresql AND mssql!

[valqk]

Hello everyone,
after losing about 3 hours debugging this issue I've found a SERIOS bug in the
insert_id function of rcube_db.inc.

I don't know who wrote it but please whoever is responsible for this to fix it.

what is broken?

see this code:

Code Select Expand

function insert_id($sequence = '')
  {
  if (!$this->db_handle || $this->db_mode=='r')
   return FALSE;
  switch($this->db_provider)
   {
   case 'pgsql':
    $result = &$this->db_handle->getOne("SELECT CURRVAL('$sequence')");

   case 'mssql':
    $result = &$this->db_handle->getOne("SELECT @@IDENTITY");

    if (DB::isError($result))
     raise_error(array('code' => 500, 'type' => 'db', 'line' => __LINE__, 'file' => __FILE__,
              'message' => $result->getMessage()), TRUE, FALSE);

    return $result;

   case 'mysql': // This is unfortuneate
    return mysql_insert_id($this->db_handle->connection);

   case 'mysqli':
    return mysqli_insert_id($this->db_handle->connection);

   case 'sqlite':
    return sqlite_last_insert_rowid($this->db_handle->connection);

   default:
    die("portability issue with this database, please have the developer fix");
   }
  }

What the _fuck_ do you think will happen when the this->db_provider is pgsql and we don't have break clause or we don't return after fetching the id?!??
Well... the MSSQL fetch part will exec invalid for pg db server query SELECT @@.... and the function will fail.

sooo here is the fix :

Code Select Expand

function insert_id($sequence = '')
  {
  if (!$this->db_handle || $this->db_mode=='r')
   return FALSE;
  switch($this->db_provider)
   {
   case 'pgsql':
    $result = &$this->db_handle->getOne("SELECT CURRVAL('$sequence')");
     break;
   case 'mssql':
    $result = &$this->db_handle->getOne("SELECT @@IDENTITY");
     break;
   case 'mysql': // This is unfortunate
    $result = mysql_insert_id($this->db_handle->connection);
     break;

   case 'mysqli':
    $result = mysqli_insert_id($this->db_handle->connection);
     break;

   case 'sqlite':
    $result = sqlite_last_insert_rowid($this->db_handle->connection);
     break;

   default:
    die("portability issue with this database, please have the developer fix");
   }

    if (DB::isError($result))
     raise_error(array('code' => 500, 'type' => 'db', 'line' => __LINE__, 'file' => __FILE__,
              'message' => $result->getMessage()), TRUE, FALSE);

    return $result;

  }
Please apply this fixed code to the current tree and notify me here so I can test with latest and greatest version.

This bug affects the create user method and it's veryy bad issue.

This bug got me very nervous. I'm going to have a cigarette.
Laters,

waiting for code include confirmation.

[seansan]

You can add the suggested change as a bug to:

http://trac.roundcube.net/trac.cgi/report

Maybe be handy in a diff file

[MichaelZicks]

Hello everyone,
after losing about 3 hours debugging this issue I've found a SERIOS bug in the
insert_id function of rcube_db.inc.

I don't know who wrote it but please whoever is responsible for this to fix it.

what is broken?

see this code:

Code Select Expand


function insert_id($sequence = '')
  {
  if (!$this->db_handle || $this->db_mode=='r')
   return FALSE;
  switch($this->db_provider)
   {
   case 'pgsql':
    $result = &$this->db_handle->getOne("SELECT CURRVAL('$sequence')");

   case 'mssql':
    $result = &$this->db_handle->getOne("SELECT @@IDENTITY");

    if (DB::isError($result))
     raise_error(array('code' => 500, 'type' => 'db', 'line' => __LINE__, 'file' => __FILE__,
              'message' => $result->getMessage()), TRUE, FALSE);

    return $result;

   case 'mysql': // This is unfortuneate
    return mysql_insert_id($this->db_handle->connection);

   case 'mysqli':
    return mysqli_insert_id($this->db_handle->connection);

   case 'sqlite':
    return sqlite_last_insert_rowid($this->db_handle->connection);

   default:
    die("portability issue with this database, please have the developer fix");
   }
  }


What the _fuck_ do you think will happen when the this->db_provider is pgsql and we don't have break clause or we don't return after fetching the id?!??
Well... the MSSQL fetch part will exec invalid for pg db server query SELECT @@.... and the function will fail.

sooo here is the fix :

Code Select Expand


function insert_id($sequence = '')
  {
  if (!$this->db_handle || $this->db_mode=='r')
   return FALSE;
  switch($this->db_provider)
   {
   case 'pgsql':
    $result = &$this->db_handle->getOne("SELECT CURRVAL('$sequence')");
     break;
   case 'mssql':
    $result = &$this->db_handle->getOne("SELECT @@IDENTITY");
     break;
   case 'mysql': // This is unfortunate
    $result = mysql_insert_id($this->db_handle->connection);
     break;

   case 'mysqli':
    $result = mysqli_insert_id($this->db_handle->connection);
     break;

   case 'sqlite':
    $result = sqlite_last_insert_rowid($this->db_handle->connection);
     break;

   default:
    die("portability issue with this database, please have the developer fix");
   }

    if (DB::isError($result))
     raise_error(array('code' => 500, 'type' => 'db', 'line' => __LINE__, 'file' => __FILE__,
              'message' => $result->getMessage()), TRUE, FALSE);

    return $result;

  }

Please apply this fixed code to the current tree and notify me here so I can test with latest and greatest version.

This bug affects the create user method and it's veryy bad issue.

This bug got me very nervous. I'm going to have a cigarette manufactured by e cigaretettes manufacturers.
Laters,

waiting for code include confirmation.

Thanks for sharing out the code.. I will apply it and will try to solve out the bug..

[alec]

You should update immediately and don't use so old version of Roundcube.