sessions race condition

[jimmy@…]

Regarding this piece of code in django/contrib/sessions/models.py:

        while 1:
            session_key = md5.new(str(random.randint(0, sys.maxint - 1)) + str(random.randint(0, sys.maxint - 1)) + settings.SECRET_KEY).hexdigest()
            try:
                self.get(session_key=session_key)
            except self.model.DoesNotExist:
                break
        return session_key

There is a very very small chance that a race condition exists between finding a uniq session, and saving it; which would result in one user ending up with a session owned by someone else. I know the chance is very small, but I do worry about it. Maybe it would be possible to also include remote_addr into the to be hashed string?

I also want to add that it would be nice to make a configuration option to make it impossible to use a session from another remote_addr. I might be to paranoid.

[James Bennett]

I'm not certain that it's something worth worrying about, given the extreme improbability -- unless I'm misreading, it would require two users to get the same pair of random numbers in the same order within an extremely short time of one another. And if we're going to worry about that probability -- my rough calculation is that such a collision will happen roughly once in every 4.6 quintillion key generations -- then we're stuck with no solution; there are also infinitesimally small chances that two users behind the same NAT would get the same pair of random numbers (in which case REMOTE_ADDR is no good) or would get the same datetime stamp from different server processes (in which case using a timestamp as salt is no good), etc.

Or is there something I'm missing here?

[jimmy@…]

If your calculation is correct, I won't have to worry about it indeed.

I blame my lack of knowledge on the exact working of the PRNG in use, it's used seed, ... because it will be very dependant on that.

Your statement on remote_addr is also correct, another one would be the chanse that md5 collisions occur while still having a random value, ...

I however believe that it would be better to make sure that in the case of a new session, save will only try and perform an INSERT, and never try the SELECT/UPDATE pair as is being used.

Maybe the flaw is not exactly in the session implementation, but in save() in general; because for whatever model we use save(), I think we can never be sure that the values will be INSERT'ed, and not UPDATE'ed by concurrent requests instead of failing and giving a chance to rollback (last time I checked).

I might for example add a Domain(model) to my Profile(model) only if it does not yet exist, concurrently with someone else, even when using sessions, both requests will perform a SELECT, one performs a SELECT and INSERT, the other one will block untill the first one is commited, if that happens, it will perform a SELECT and UPDATE, and both sessions receive an 'OK' while only one has the domain in it's profile. To make it worse, it could be possible that they both send a signal to some daemon to process the domain addition, and things go boom. Am I correct on this?

If this is correct, it would be nice to be able to force certain behaviour and take advantage in the session implementation for example.

eg:

def save(..., new_obj=False, existing_obj=False):

I thank you already for your time spend.

[anonymous]

I also noticed this piece of code a few weeks ago and thought the same thing.

However, this is not the only piece of code that has a race condition. There's e.g. get_or_create() where a race condition is very probable.

See my ​post to the django-developers mailing list for details.

[Thomas Steinacher <tom@…>]

(the previous comment was by me)

[James Bennett]

Closing this "worksforme" for the various reasons outlined above.

Thomas, as pointed out in that thread, the solution for get_or_create in critical sections is to use transaction isolation.