Fix instances of mutable default arguments to functions/methods

Bug #1307878 reported by Brian Cline
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Glance
Fix Released
Undecided
Brian Cline

Bug Description

In a few points throughout the codebase, mutable lists and mutable dicts are being used as default function/method arguments.

In Python, this is an issue since functions are treated as objects that can maintain state between calls. As a result, this only gets set once, and it's possible for it to stack list values over time in cases when you might expect them to be empty. Depending on use, this can cause incredibly complex and yet very subtle bugs in code that reads just fine. In Glance's case, since a few instances of this are in several ACL-related methods in glance.store.*, there is *potential* for security concern (not confirmed).

Here's some additional information illustrating and explaining this behavior in Python:
http://effbot.org/zone/default-values.htm
http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument

There are no comments in the code I've seen that indicate this usage is meant specifically to take advantage of this subtlety in the language. We'd definitely want to document that if it is the case.

Wanted to create this as a discussion point if needed, and as a courtesy to attach it to the patch I'm going to push in a few minutes. The full test suites seem to pass locally, so will be curious what Jenkins has to say.

Revision history for this message
Brian Cline (briancline) wrote :

For some reason recent patch sets aren't being automatically annotated here.

This is the patch pushed for this issue:
https://review.openstack.org/87475

Changed in glance:
assignee: nobody → Brian Cline (briancline)
Revision history for this message
Openstack Gerrit (openstack-gerrit) wrote : Fix merged to glance (master)

Reviewed: https://review.openstack.org/87475
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=bebe906ee7ddcc8785c927b559c930d62e972cbb
Submitter: Jenkins
Branch: master

commit bebe906ee7ddcc8785c927b559c930d62e972cbb
Author: Brian Cline <email address hidden>
Date: Tue Apr 15 02:10:28 2014 -0500

    Uses None instead of mutables for function param defaults

    Addressing bug 1307878, changes use of mutable lists and dicts as
    default arguments and defaults them within the function. Otherwise,
    those defaults can be unexpectedly persisted with the function between
    invocations and erupt into mass hysteria on the streets.

    To my knowledge there aren't known cases of the current use causing
    specific issues, but needs addressing (even stylistically) to avoid
    problems in the future -- ones that may crop up as extremely subtle or
    intermittent bugs...or worse, security vulnerabilities.

    In Glance's case there are ACL-related methods using this, so
    although I haven't confirmed one way or the other yet, I've marked it
    with SecurityImpact so that a more knowledgeable set of eyes can
    review it in this context as well.

    Closes-Bug: #1307878
    SecurityImpact

    Change-Id: Ic17c330eff701ff63701c0029b26db7093a1d73d

Changed in glance:
status: New → Fix Committed
Thierry Carrez (ttx)
Changed in glance:
milestone: none → juno-1
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in glance:
milestone: juno-1 → 2014.2
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.