[Rt-devel] [PATCH] Fix for SetOwner race condition

[Todd Chapman]

We are a pretty high volume RT user. Our customer service
people need to chug through so many tickets that I even
had to customize RT so that when a ticket is resolved the
next ticket in the queue is automatically taken and
displayed.

Several times in one week 2 people we able to Take the
same ticket. What is up with that?

Armed with 2 debugger sessions I stepped through the
code for SetOwner and was able to figure out that
the decision to allow a user to take a ticket was
happening outside of the critical section of code. This
means that the decision to allow a user to take a ticket
may be invalidated between the time the decision was
made and the time the ticket was actually taken.

By moving around the locking and a few other tricks
I was able to eliminate the race condition.
We have been using this new code for about 2 weeks with
no problems.

Patch is attached.

-Todd
-------------- next part --------------
==== Patch <set_owner_race_condition> level 1
Source: e4b3ae24-6623-0410-94c5-e08a22d88352:/local/bp/3.6-SetOwner_fix:7136
Target: e417ac7c-1bcc-0310-8ffa-8f5827389a85:/rt/branches/3.6-RELEASE:6920
        (svn://svn.bestpractical.com)
Log:
 r7135 at fe-rtr00a:  todd-chapman | 2007-02-07 10:20:36 -0500
 Submitting patch for SetOwner race condition.
 r7136 at fe-rtr00a:  todd-chapman | 2007-02-07 10:36:00 -0500
 Fixed race condition in SetOwner.

=== lib/RT/Ticket_Overlay.pm
==================================================================
--- lib/RT/Ticket_Overlay.pm	(revision 6920)
+++ lib/RT/Ticket_Overlay.pm	(patch set_owner_race_condition level 1)
@@ -2979,14 +2979,28 @@
     my $NewOwner = shift;
     my $Type     = shift || "Give";
 
+    $RT::Handle->BeginTransaction();
+
+    $self->_SetLastUpdated(); # lock the ticket
+    $self->Load( $self->id ); # in case $self changed while waiting for lock
+
     my $OldOwnerObj = $self->OwnerObj;
 
+    my $NewOwnerObj = RT::User->new( $self->CurrentUser );
+    $NewOwnerObj->Load( $NewOwner );
+    unless ( $NewOwnerObj->Id ) {
+        $RT::Handle->Rollback();
+        return ( 0, $self->loc("That user does not exist") );
+    }
+
+
     # must have ModifyTicket rights
     # or TakeTicket/StealTicket and $NewOwner is self
     # see if it's a take
     if ( $OldOwnerObj->Id == $RT::Nobody->Id ) {
         unless (    $self->CurrentUserHasRight('ModifyTicket')
                  || $self->CurrentUserHasRight('TakeTicket') ) {
+            $RT::Handle->Rollback();
             return ( 0, $self->loc("Permission Denied") );
         }
     }
@@ -2997,27 +3011,24 @@
 
         unless (    $self->CurrentUserHasRight('ModifyTicket')
                  || $self->CurrentUserHasRight('StealTicket') ) {
+            $RT::Handle->Rollback();
             return ( 0, $self->loc("Permission Denied") );
         }
     }
     else {
         unless ( $self->CurrentUserHasRight('ModifyTicket') ) {
+            $RT::Handle->Rollback();
             return ( 0, $self->loc("Permission Denied") );
         }
     }
 
-    my $NewOwnerObj = RT::User->new( $self->CurrentUser );
-    $NewOwnerObj->Load( $NewOwner );
-    unless ( $NewOwnerObj->Id ) {
-        return ( 0, $self->loc("That user does not exist") );
-    }
-
-    # If we're not stealing and the ticket has an owner and it's not 
+    # If we're not stealing and the ticket has an owner and it's not
     # the current user
     if ( $Type ne 'Steal' and $Type ne 'Force'
          and $OldOwnerObj->Id != $RT::Nobody->Id
          and $OldOwnerObj->Id != $self->CurrentUser->Id )
     {
+        $RT::Handle->Rollback();
         return ( 0, $self->loc("You can only take tickets that are unowned") )
             if $NewOwnerObj->id == $self->CurrentUser->id;
         return (
@@ -3028,17 +3039,17 @@
 
     #If we've specified a new owner and that user can't modify the ticket
     elsif ( !$NewOwnerObj->HasRight( Right => 'OwnTicket', Object => $self ) ) {
+        $RT::Handle->Rollback();
         return ( 0, $self->loc("That user may not own tickets in that queue") );
     }
 
     # If the ticket has an owner and it's the new owner, we don't need
     # To do anything
     elsif ( $NewOwnerObj->Id == $OldOwnerObj->Id ) {
+        $RT::Handle->Rollback();
         return ( 0, $self->loc("That user already owns that ticket") );
     }
 
-    $RT::Handle->BeginTransaction();
-
     # Delete the owner in the owner group, then add a new one
     # TODO: is this safe? it's not how we really want the API to work
     # for most things, but it's fast.
@@ -3073,8 +3084,6 @@
         return ( 0, $self->loc("Could not change owner. ") . $msg );
     }
 
-    $RT::Handle->Commit();
-
     ($val, $msg) = $self->_NewTransaction(
         Type      => $Type,
         Field     => 'Owner',
@@ -3086,9 +3095,14 @@
     if ( $val ) {
         $msg = $self->loc( "Owner changed from [_1] to [_2]",
                            $OldOwnerObj->Name, $NewOwnerObj->Name );
+    }
+    else {
+        $RT::Handle->Rollback();
+        return ( 0, $msg );
+    }
 
-        # TODO: make sure the trans committed properly
-    }
+    $RT::Handle->Commit();
+
     return ( $val, $msg );
 }
 

==== BEGIN SVK PATCH BLOCK ====
Version: svk 1.08 (linux)

eJytVktr5EYQ1i3LEHLJKbfG1mIbVjNS6zEPx8Psbmw2xMTLeDaHhDC0Wq1R2xpJtHr8wDL4weSQ
X2AILAkhJIeQew7J78gtfyXVPQ/bJGZ3IWKQW11V31dVXV3lHTHY7DlVt2tXpmNX+1981um8JJIm
jx2vMoOKRVzmwvSrlB2x1HSrNB+ZXpWRMQNpmU8EVQtJxIhJteD0kMlu1wG41gxuW0MsYDVqSGSe
lWZbww+lYMx0Kq/nVz1X/Yam065KBhINOxTsiJc8z4C46bgBaIC6A+Z5wbKhyHMJkqDtBT2sbO2K
pnnJhgocdgAW1LEJAWn9iAtGwaNTFQ0PNdTcXFP7c/NbPdBY4rj/gRMonEZ/oGneBIWXUN4igpin
zMTeHKUx0Dkc7h0xkZLTejHWtG/CdWe4WkqKIj0dSnYiI5ZKorlcXPmUNiMaunEchq5HCG2Stu1E
Drb9OGzh2ISVqoHPDeOb79Xvy2nDmL5eMS4ud4zL96c/rV58bVzh6etPboz9q78c4+rD6Q8f3xiX
q9Mfn822/jQue9OfnRvjujP9JQGRmP760UVsfPvV9Lcj4yqa/j6+KIy/D6Z/fGD2B53OC5JFKbO6
z9iIZwNBspJQCUe9vrFZqyF4zJKlsdUd7jO5S0r5qoiIZBGI0SpKc3qIZMLQrOru6u/mJFpffPAI
aX2eIUpKNttGNCHZiEXoOIH0o2PCJc9GKM6Fxp2xj0+RuZdGe8cZE3vhAdpaQC52Nu9F0c/TNCT0
UHmP5o9gciIytI7sJwtjwF9fGSREoknJBIpyVqIsl4id8FKubKC59XntIfB33f80RsdsTTDNUkpG
UhUq6N3JHkpICVsoV5FpGZdr2q//y4t3SAlJBSPRqXKmBB9he+bkneTMMqTeLIUzPVuivxv9uBzd
5rv2L/vn+XjMpbKG63Az+i5/7/rRxfUjWBpCsEy+Am+trnpDMaxvPFmSqKfPR4nUq60uWjGF+ly5
o7Gh16rSgXoVnZ/rP2dnZ6ichOgFKWcAtdpWAtnAyx3wc0AOoWgIKoggYzi5BB1zmejjJFIKHk4k
yNe0+po+zbWXgmeUFyRdA9qFhCuQWW6tkkI3gpKAtjEhKdLuoljkY6Qy8vT5NlKGtzDaONNClQCU
hwfQjVQS+zrDJXIQj7VPxcJIV5na0ej1peYki1istKHe6hAwnQDQvSycLS+kvr1bqEx4LOHket7d
pq976bzfUa/tuxSHmDXjIHAdN3T9tue3cRBQtxVj3fqxjSskYK74vZhZQgrbJh2EZB5FFrSIYgwh
VggaY9OysWU3kWN3sN1xA2TZvm3X0P4khBLRzaNQI063EGhXukUgQShDNM9gFkFbq9c0VfD2VG7Q
se0F1Q4/gQO6D6m62oKtXlNjRE1OfYuHSnO41Ox2cWViPJvKAz1a4eQyDnOmJOnjoIIZAxEkMBdh
4sLHZMIjEwdVA24nSRth0XDrgbUgg1SfzCey6cJ/Cl7oEoY9Kwiwa9meY1ttj/oWs1sE46jVcn3c
3cDVgzxe1RCyEcIMoAkrNVV/e3f76f62nn3YnrE4TZhb1HJCSi3bBZZWHBN4+S3cdFtt0vLNoPVW
3nQejqujovoHLmPZuw==
==== END SVK PATCH BLOCK ====