Race condition in QuerySet.update_or_create()

[Michael Sanders]

I believe that there is a potential race condition in QuerySet.update_or_create()

When initially trying to obtain the object to update using get(), the row is locked using the select_for_update() method - this would lock the object against changes by other processes until the end of the transaction. However, if the object does not already exist, update_or_create() then calls the _create_object_from_params() method - see code:

      with transaction.atomic(using=self.db):
          try:
              obj = self.select_for_update().get(**lookup)
          except self.model.DoesNotExist:
              obj, created = self._create_object_from_params(lookup, params)

The _create_object_from_params() method looks like this:

 def _create_object_from_params(self, lookup, params):
     """
     Try to create an object using passed params. Used by get_or_create()
     and update_or_create().
     """
     try:
         with transaction.atomic(using=self.db):
             params = {k: v() if callable(v) else v for k, v in params.items()}
             obj = self.create(**params)
         return obj, True
     except IntegrityError as e:
         try:
             return self.get(**lookup), False
         except self.model.DoesNotExist:
             pass
         raise e

Here, it initially tries to create the object. However (assuming that the object has a unique key constraint policed by the DB), if the object has already been created meanwhile (by a different process) the IntegrityError is caught and the code uses the get() method to attempt to obtain the newly created object. The object is then returned to the calling update_or_create() method to update - however in this case, the row has not been locked against changes. So, at this point other processes are free to modify the DB row and those changes might then be overwritten by the save() in update_or_create().

Suggested Fix

This could be fixed by changing the _create_object_from_params() method to take a new parameter to specify whether the object should be locked on get, i.e.:

 def _create_object_from_params(self, lookup, params, lock=False):
     """
     Try to create an object using passed params. Used by get_or_create()
     and update_or_create().
     """
     try:
         with transaction.atomic(using=self.db):
             params = {k: v() if callable(v) else v for k, v in params.items()}
             obj = self.create(**params)
         return obj, True
     except IntegrityError as e:
         try:
             if lock:
                 return self.select_for_update().get(**lookup), False
             else:
                 return self.get(**lookup), False
         except self.model.DoesNotExist:
             pass
         raise e

The call to _create_object_from_params() from get_or_create() would remain the same, but from update_or_create() it would change to:

             obj, created = self._create_object_from_params(lookup, params, lock=True)

[Simon Charette]

The issue is legitimate and the suggested fix makes sense; if update_or_create enforces select_for_update() when a row exists it should always do so.

Would you be able to submit a PR on Github with some tests. I guess this could qualify for backports as it's a possible data loss issue.

[Michael Sanders]

I am obtaining permission from my employer and then will create a PR.

[Michael Sanders]

Permission from my employer has been obtained, so I will now work on a PR.

[Michael Sanders]

PR: ​https://github.com/django/django/pull/10252

[Tim Graham]

Do you think we should backport to 1.11 based on your comment about data loss?

[Simon Charette]

I think it should be pretty safe to backport for the rare cases when this happens. It's really an edge case but as Michael demonstrated in his test it can effectively lead to data-losses.

[Tim Graham <timograham@…>]

In 271542da:

Fixed #29499 -- Fixed race condition in QuerySet.update_or_create().

A race condition happened when the object didn't already exist and
another process/thread created the object before update_or_create()
did and then attempted to update the object, also before update_or_create()
saved the object. The update by the other process/thread could be lost.

[Tim Graham <timograham@…>]

In 221ef69a:

[2.1.x] Fixed #29499 -- Fixed race condition in QuerySet.update_or_create().

A race condition happened when the object didn't already exist and
another process/thread created the object before update_or_create()
did and then attempted to update the object, also before update_or_create()
saved the object. The update by the other process/thread could be lost.

Backport of 271542dad1686c438f658aa6220982495db09797 from master

[Tim Graham <timograham@…>]

In 44418260:

[2.0.x] Fixed #29499 -- Fixed race condition in QuerySet.update_or_create().

A race condition happened when the object didn't already exist and
another process/thread created the object before update_or_create()
did and then attempted to update the object, also before update_or_create()
saved the object. The update by the other process/thread could be lost.

Backport of 271542dad1686c438f658aa6220982495db09797 from master

[Tim Graham <timograham@…>]

In 2668418d:

[1.11.x] Fixed #29499 -- Fixed race condition in QuerySet.update_or_create().

A race condition happened when the object didn't already exist and
another process/thread created the object before update_or_create()
did and then attempted to update the object, also before update_or_create()
saved the object. The update by the other process/thread could be lost.

Backport of 271542dad1686c438f658aa6220982495db09797 from master

[Tim Graham <timograham@…>]

In 8a0b9051:

[1.11.x] Refs #29499 -- Skipped QuerySet.update_or_create() test that fails on MySQL.