Opened 6 years ago

Closed 13 months ago

#28948 closed Bug (fixed)

CookieStorage performance issues

Reported by: Michal Čihař Owned by: David Wobrock
Component: contrib.messages Version: 2.0
Severity: Normal Keywords:
Cc: Adam Johnson, David Wobrock Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The CookieStorage tries to generate as big cookie as possible to fit all messages. However doing this on lot of small messages is very expensive and can take several seconds on the server, potentially leading to denial of service.

Here is simple code to reproduce the slowness:

#!/usr/bin/env python

# Configure needed settings
from django.conf import settings
settings.configure(MESSAGE_TAGS={})

from django.contrib.sessions.middleware import SessionMiddleware
from django.contrib.messages.middleware import MessageMiddleware
from django.contrib.messages.storage.cookie import CookieStorage
from django.contrib.messages.api import info
from django.http.request import HttpRequest
from django.http.response import HttpResponse
from django.contrib.messages.storage import default_storage

# Request and response objects
response = HttpResponse()
request = HttpRequest()

# Process request by middleware
SessionMiddleware().process_request(request)
mm = MessageMiddleware()
mm.process_request(request)

# Insert messages
for x in range(500):
    info(request, 'm:{0}'.format(x))

# Measure response processing time
import timeit
print(timeit.timeit(
    'mm.process_response(request, response)',
    globals=globals(), number=10
))

In my case the DOS was triggered by broken client who repeatedly posted form generating message, but never did follow redirect to display the messages, so nothing really sophisticated.

Quickly looking at the code following performance improvements come to my mind:

  • Avoid repeated encoding of the messages, encode them all at once and then operate on encoded strings
  • Avoid calculating HMAC while calculating length as length of it is fixed
  • Do bisect instead of removing messages one by one

Change History (9)

comment:1 by Adam Johnson, 6 years ago

Cc: Adam Johnson added

comment:2 by Sergey Fedoseev, 6 years ago

Cc: Sergey Fedoseev added

comment:3 by Tim Graham, 6 years ago

Triage Stage: UnreviewedAccepted

comment:6 by Sergey Fedoseev, 3 years ago

Cc: Sergey Fedoseev removed

comment:7 by David Wobrock, 14 months ago

Cc: David Wobrock added
Has patch: set
Owner: changed from nobody to David Wobrock
Status: newassigned

comment:8 by Mariusz Felisiak, 13 months ago

Triage Stage: AcceptedReady for checkin

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 13 months ago

In 9d0c878a:

Refs #28948 -- Precomputed once serialized cookie messages.

When the cookie size is too long, the same messages were serialized
over and over again.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 13 months ago

In 21757bbd:

Refs #28948 -- Removed superfluous messages from cookie through bisect.

comment:11 by Mariusz Felisiak, 13 months ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top