moderation can potentially discard vote under simultaneous submi

I may have figured out why my math is bad...it looks like the score is an average of the users votes. However, there is a race condition. Here are lines 1312 to 1328 from modules/comment.module:

            $comment = db_fetch_object(db_query("SELECT * FROM comments WHERE cid = '%d'", $cid));
            $users = unserialize($comment->users);
            if ($user->uid != $comment->uid && !(comment_already_moderated($user->uid, $comment->users))) {
              $users[$user->uid] = $vote;
              $tot_score = 0;
              foreach ($users as $uid => $vote) {
                if ($uid) {
                  $tot_score = $tot_score + $votes[$vote];
                }
                else {
                  // vote 0 is the start value
                  $tot_score = $tot_score + $vote;
                }
              }
              $new_score = round($tot_score / count($users));
              db_query("UPDATE comments SET score = '$new_score', users = '%s' WHERE cid = '%d'", serialize($users), $cid);

Notice that the math is done outside of the database before the update. As far as I can tell, the rows being updated have not yet been locked (I'm not even sure if there are any transactional boundaries anywhere in the whole application). If two users moderate at the same time, one of them is at risk of not having his moderation "take" (i.e., it looks like it will appear as if that comment was never moderated by one of the users).

---

If possible the calculation should be made in the SQL statement (or at least the row should be locked within a transaction boundary before reading the old score). Here's an example using PEAR_DB (one would have to add another column num_users to the table if one still wanted to calculate averages):

    // Prepare the statement (probably belongs in a define somewhere)
    $st = &$db->prepare('UPDATE comments
        SET score = ((score * num_users) + ?) / (num_users + 1.0),
        num_users = num_users + 1,
        users = ?
        WHERE cid = ?');

    // Assign the values
    if (!DB::isError($st))
    {
        $err = &$db->execute($st, array($vote, serialize($users), $cid));
    }
    else
    {
        $err = &$st;
    }

    // Deal with $err
    if (DB::isError($err))
    {
        ...
    }

I would also leave score as a float (rather than an integer) to reflect its nature as an average rather than an aggregate. Without it being a float, even the current method of calculating scores is wrong (or at least really lossy after two votes).

You'll probably note that even with my proposed SQL statement, this doesn't help out in the case where a single malicious user makes several concurrent attempts to moderate the same comment in efforts to gain more influence. Now that I think about it, my half-assed proposal seems worse...one runs the risk of counting votes without properly attributing them to anyone, where at least in the original code, the worst that can happen is that the net effect of someone's moderation is zero (i.e., they have to do it again). I don't think there's there's a way around that without transactional row-locking given the current data model, since it would be impossible (or at least really really difficult) to do the parsing and evaluation of the users field inside a WHERE clause.

Of course, all this makes me wonder how many other non thread-safe things there are in the application....