user_save() may be called as an API function by various modules. However, there is no failure mode if the user UPDATE/INSERT query fails. This could be extremely bad. For example on INSERT, on postgres subsequent actions in hook_user may be carried out on a user object with the previously inserted user's uid. On MySQL it's even worse - the uid may be something quite random, because of the way db_last_insert_id('users', 'uid'); is implemented.

An example of how this could happen - the "name" column is unique. A module that didn't properly validate the data it's trying to save could attempt to insert a user with a duplicate name. This query would fail, but hook_user would still be called with an invalid uid.

This risk is in part due to the abolition of the sequences table, so was not present in Drupal 5.x.

Note - this came out of a discussion regarding this issue: http://drupal.org/node/164532 since adding the unique index to the mail column made this concern more apparent (as first noted by ajk^).
Attached patch changes user_save() to check for failed UPDATE/INSERT queries. Note the user will already see an SQL error message on the screen.

CommentFileSizeAuthor
#23 user_save-abort-204705-15.patch5.08 KBgábor hojtsy
#15 user_save-abort-204705-14.patch4.96 KBpwolanin
#2 user_save-abort-204705-2.patch1.9 KBpwolanin
user_save-abort-0.patch2.23 KBpwolanin

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

- You have some index changes included in the patch? Are these intended to be here?

- How expensive can user_save() get? I mean, why don't we do the usual "SELECT, if not found INSERT" approach? I fully understand the additional SELECT adds a bit of an overhead, but we have the uid indexed already, so it shoud be "pretty quick" (oh, that always sounds dangerous to say :) Anyway, I'd rather see a proper SQL error-ess recovery, then realizing that "oh, we had an SQL error, so although it was already displayed/logged, let's quickly retract now". IMHO we should prevent the SQL error.

pwolanin’s picture

pwolanin commented
StatusFileSize
new user_save-abort-204705-2.patch1.9 KB

ops - yeah the index changes were part of the other issue.

I'm not sure it's worth running an extra query - with that approach, we'll have to add an additional query for each new unique index. In addition, such a query is already run in the form validation, so this is really to protect against an API call where the data was not properly validated.

See: http://api.drupal.org/api/function/_user_edit_validate/6

New patch attached without index changes, also adds to doxygen.

moshe weitzman’s picture

moshe weitzman commented

this could be said for many queries like nodes, comments, etc. i am not so sure this is a good move. anyway, it seems that this ought to be done in drupal_write_record since thats the master insert of them all.

pwolanin’s picture

pwolanin commented

@moshe - this is more specifically a problem with {users}, because there is a unique index on name. Inserts or updates to {node} and {comment} can't fail in this way.

yched’s picture

yched commented

I don't think this invalidates moshe's suggestion. drupal_write_record uses the schema to do the writing. If special checking / error reporting is needed whenever unique keys are involved, it probably belongs there.

pwolanin’s picture

pwolanin commented

@yched - ok, but that would need to be done in addition- should we address that in a separate issue? user_save() does not use drupal_write_record, so I think this patch is still appropriate.

yched’s picture

yched commented

user_save() does not use drupal_write_record
Ah, true, missed that :-). Then it's probably different... I don't think moving user_save to drupal_write_record would be a good move now (maybe it has even been explicitly postponed to D7 already), so I guess a separate fix is needed. This is not a proper review of the patch, though :-)

About drupal_write_record, then : fixing it might not be *required* for the uses in curent core, but maybe there will be cases in contrib ?

moshe weitzman’s picture

moshe weitzman commented

I don't want to be a pest, but there is a perfectly good patch in the queue that makes user_save() use drupal_write_record(). that might be preferable to a special case here. i'm not strongly opinionated though.

pwolanin’s picture

pwolanin commented

@moshe - link? Also, is someone going to rewrite drupal_write_record() to check for unique indices?

catch’s picture

catch
he/him
Primary language English
commented

@pwolanin: I think this is it: http://drupal.org/node/181411#comment-644587

pwolanin’s picture

pwolanin commented

ok - well since that patch is moved to D7, we still need something like this for D6.

chx’s picture

chx commented

Decisions can be changed -- if that patch is needed for a clean soluton here and also elsewhere then we can move it back to the D6 queue.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

So far what I have seen is that drupal_write_record() would have the same problem anyway. While it introduced problems with code around the node saving and other parts of Drupal, which took quite some time and hair tearing to fix, so I'd rather not rush to use it.

chx’s picture

chx commented

we could fix up drupal_write_record to check for unique keys and this would be solved for all involved.

pwolanin’s picture

pwolanin commented
StatusFileSize
new user_save-abort-204705-14.patch4.96 KB

quick re-roll - functions calling user_save() should check that the return is not FALSE;

note - this is already a bug fix (maybe should be backported) - since user_save already calls user load:

$user = user_load(array('uid' => $array['uid']));

so if the save failed, user_save() will already be returning FALSE:

function user_load($array = array()) {
    ...
  if ($user = db_fetch_object($result)) {
    ...
  }
  else {
    $user = FALSE;
  }

  return $user;
}
gerhard killesreiter’s picture

gerhard killesreiter commented

I am not sure the drupal_set_message calls make a lot of sense. We don't add similar calls elsewhere. We should however add a watchdog call in user_save in case of failure.

pwolanin’s picture

pwolanin commented

@Gerhard - the drupal_set_message calls are all in the context of form _submit functions which make other such calls. However, I don't feel strongly either way.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

pwolanin: Gerhard still makes a good point about wachdog() logging the bad call, so these kinds of misbehaviors can be noted in the log.

pwolanin’s picture

pwolanin commented

@Gabor - such an SQL error will already cause an entry in the log - do you think an additional log entry is required?

http://api.drupal.org/api/function/_db_query/6
http://api.drupal.org/api/function/drupal_error_handler/6

AjK’s picture

AjK commented

Applied patch to latest head and all was well, did what it said on the tin.

I have to say that this seems a sensible idea to me. Having user_save() must return a user object no matter what doesn't allow for a failure condition. The idea of returning FALSE on fail is deff a +1 from me.

hswong3i’s picture

hswong3i commented
Status: Needs review » Needs work

Just hope to confirm: is patch from http://drupal.org/node/207170 able to overcome the concern in #13 and #14? Since drupal_write_record() is now return FALSE if facing error during INSERT/UPDATE query, maybe the user_save() error checking can become simpler? E.g. don't use manual INSERT/UPDATE queries, but drupal_write_record() so come with error checking?

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review

@hswong31 - user_save() does NOT use drupal_write_record(), and it was the clear decision to postpone any such change until D7.

To me the only outstanding question with this patch is whether or not to remove the drupal_set_message() calls and/or replace them with watchdog() calls.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Fixed
StatusFileSize
new user_save-abort-204705-15.patch5.08 KB

All right. I don't think this patch is exactly nice, but it is a workable stop-gap solution for now, until Drupal 7 introduces more drupal_write_record()s. The error is already logged in watchdog with the DB error logging, and in user facing operations, we note the user about the error.

I did a small change on the patch though: moved the db_last_insert_id() call to after the error detection, as we don't need to call that if there is an error. Committed the attached patch.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.