Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race conditions for parallel requests due to cache #371

Closed
ThiefMaster opened this issue Mar 11, 2019 · 17 comments

Comments

@ThiefMaster
Copy link
Contributor

commented Mar 11, 2019

I just noticed that something in webargs or marshmallow isn't thread-safe. Take this minimal example"

import time

from flask import Flask, jsonify, request
from marshmallow.fields import Field
from webargs.flaskparser import use_kwargs


app = Flask(__name__)


class MyField(Field):
    def _serialize(self, value, attr, obj):
        return value

    def _deserialize(self, value, attr, data):
        print 'deserialize', request.json, value
        time.sleep(0.25)
        return value


@app.route('/test', methods=('POST',))
@use_kwargs({
    'value': MyField(),
})
def test(value):
    time.sleep(1)
    return jsonify(webargs_result=value, original_data=request.json['value'])

Run it with threading enabled:

$ FLASK_APP=webargsrace.py flask run -p 8080 --with-threads

Now send two requests in parallel, with different values:

$ http post http://127.0.0.1:8080/test 'value=foo' & ; http post http://127.0.0.1:8080/test 'value=bar' &

The output from these two requests is:

{
    "original_data": "bar",
    "webargs_result": "bar"
}
{
    "original_data": "foo",
    "webargs_result": "bar"
}

Clearly not what one would have expected! 💣

The output of the print statement showing the request data and what the field receives confirms the issue:

deserialize {u'value': u'bar'} bar
deserialize {u'value': u'foo'} bar

Tested with the latest marshmallow/webargs from PyPI, and also the marshmallow3 rc (marshmallow==3.0.0rc4, webargs==5.1.2).

@ThiefMaster

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

I think this is caused by the fact that self._cache on the Parser is not thread-safe.

@ThiefMaster ThiefMaster changed the title Race conditions for parallel requests Race conditions for parallel requests due to cache Mar 11, 2019

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Thanks for the report and the test case.

Indeed your test passes when removing the caching mechanism.

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Thanks for reporting @ThiefMaster .

I won't have time to work on this for the next few days, but I'd gladly review/merge/release a patch. @ThiefMaster @lafrech Are either you up for this?

@ThiefMaster

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

I'm looking into fixing it right now

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I'm investigating.

Removing the cache fixes the issue. I'm trying to see if there is a simple means to keep a cache with a scope restricted to the request.

For the record, the cache was proposed in #23 and introduced in 243451a.

@ThiefMaster

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

My approach would be passing around the cache explicitly (and creating the cache dict in parse() so it's never shared).

How much of the parsing-related functions are considered part of the public API? Adding a new cache arg that's passed around - even if it's optional - would break code containing custom location handlers, either by due to the extra arg or (when using inspection to pass the arg only if if the method accepts it) due to self._cache being gone...

@ThiefMaster

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

I added a possible fix in #372

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

My approach would be passing around the cache explicitly (and creating the cache dict in parse() so it's never shared).

Yes. Here's a quick draft, FlaskParser only: https://github.com/marshmallow-code/webargs/commits/fix_cache_race_condition.

You just beat me to it, with a patch covering all parsers.

How much of the parsing-related functions are considered part of the public API? Adding a new cache arg that's passed around - even if it's optional - would break code containing custom location handlers, either by due to the extra arg or (when using inspection to pass the arg only if if the method accepts it) due to self._cache being gone...

I'm afraid the change will be a breaking one.

We could also release a fix on the 5.x branch that would only disable the cache.

@ThiefMaster

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

We could also release a fix on the 5.x branch that would only disable the cache.

The problem I see there is that this will re-parse the json body for every single field in the schema. However, I think we could fix it for flask by storing it as an attribute on the flask req context instead of using the cache...

@ThiefMaster

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Thinking about this again, why don't we create a new Parser instance whenever we parse something? That way self._cache is safe and the changes needed are minimal.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

We could also release a fix on the 5.x branch that would only disable the cache.

The problem I see there is that this will re-parse the json body for every single field in the schema. However, I think we could fix it for flask by storing it as an attribute on the flask req context instead of using the cache...

Sure. I can't comment about the performance impact. I suppose it would be more important in Tornado, as the cache was introduced for Tornado in the first place.

Anyway, that would be a fallback solution, until user code is migrated to webargs 6.0.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Thinking about this again, why don't we create a new Parser instance whenever we parse something? That way self._cache is safe and the changes needed are minimal.

I don't think this can be done in a non-breaking fashion either, but it could be worth investigating.

I suppose you mean make use_args and use_kwargs class methods that instantiate a parser on each parse. We'd lose the ability to pass default locations on init, but I don't think it is that bad. The user can subclass and override DEFAULT_LOCATIONS.

Users instantiating a Parser themselves (e.g. https://github.com/Nobatek/flask-rest-api/blob/master/flask_rest_api/arguments.py#L14) should modify their code.

@ThiefMaster

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Check #373 - this should be backwards-compatible, without losing the benefit of caching.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Nice. We could use this as a 5.x hotfix.

@sloria would you like to review and release? I think it is good to go.

I'd support issuing a breaking change / less twisted implementation 6.0 version.

I see two options:

  • Keeping current logic and passing cache all along the parsing functions. A bit clumsy and verbose, but simple.
  • Changing Parser classes into parser class factories, making use_args, use_kwargs, and perhaps parse class methods.

The latter sounds more appealing. Besides, it only breaks the code for users instantiating the parser themselves, but not for those calling use_args / use_kwargs.

@ThiefMaster

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

for 6.0 the second option indeed sounds better - passing around the cache all the time is pretty ugly

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Shall we file a CVE for this?

Everything that allows validation to be circumvented is a potential security issue. The fact that it is cross-requests makes it even worse.

Imagine a resource allowing a user to set personal info, including a password. If a user repeatedly submits information that is intentionally long to deserialize, which could be achieved if the schema contains a self-nested structure for instance, or accepts long arrays, it is likely that everyone using that same resource at the same time will end up with the password set by the long request.

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

The patch is released in 5.1.3. I've also requested a CVE ID and will report on Tidelift once that's done.

We can discuss refactoring the solution in #374 .

Thank you @ThiefMaster for the quick response on this.

@sloria sloria closed this Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.