Comment module INSERT INTO failure not checked

Hi guys,

I ran in a problem with comment_notify generating SQL errors. The errors where in link with inserting a notification note in the module table. The INSERT INTO uses the command identifier (cid) to uniquely identify the comment in the notification table.

Up to here, nothing too surprising.

After a little more research, I found out that the problem was actually really coming from pingback that attempts to save a pingback in the comment table. It does so by creating an 'edit' variable and calling the comment_save() function, which in turns calls the comment_notify_comment() hook as you would expect.

The problem comes from this code:

        db_query("INSERT INTO {comments} (nid, pid, uid, subject, comment, format, hostname, timestamp, status, thread, name, mail, homepage) VALUES (%d, %d, %d, '%s', '%s', %d, '%s', %d, %d, '%s', '%s', '%s', '%s')", $edit['nid'], $edit['pid'], $edit['uid'], $edit['subject'], $edit['comment'], $edit['format'], ip_address(), $edit['timestamp'], $edit['status'], $thread, $edit['name'], $edit['mail'], $edit['homepage']);
        $edit['cid'] = db_last_insert_id('comments', 'cid');

        // Tell the other modules a new comment has been submitted.
        comment_invoke_comment($edit, 'insert');

As you may notice, the INSERT INTO is followed by a get last ID. That last ID will be undefined if the INSERT INTO fails for whatever reason. Yet, whether it failed, the 'insert' operation of the hook_comment() functions will be called.

I think that it is required that you check that the INSERT INTO worked before invoking other modules that expect to be called only if the comment was successfully saved in the database.

I would propose that we check the 'cid' as in:

      if ($edit['cid']) {
        // Tell the other modules a new comment has been submitted.
        comment_invoke_comment($edit, 'insert');
      }

However, testing the return value of the db_query() may be wiser? (because we should probably not call db_last_insert_id() when db_query() fails.)

Also, further down you may end up calling this other hook:

  comment_invoke_comment($edit, 'publish');

which is not a good idea either if $edit['cid'] is not properly defined.

Note that to see this INSERT fail, at least with PostgreSQL, the pingback received very long names (more than 60 characters.)

Thank you.
Alexis Wilke