psf / psf/requests

should_bypass_proxies not thread-safe

Open
#4,997 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
54.3k
Forks
10.4k
Avg merge
16h 43m
Merged PRs (30d)
3

Description

requests.utils.should_bypass_proxies temporary modifies the environment, calls into httplib and then restores the environment. However, it takes no locks while doing so, so two threads both using requests could lead to a permanent change to the environment. Even with a lock, any other thread that depends on the environment may observe the change (e.g. it may launch curl in a subprocess, and curl will then pick up this no_proxy setting).

Expected Result

The environment is not modified by requests.

Actual Result

I haven't observed this bug in practice - it's a theoretical race condition I can see in the code. In the reproduction code below, I believe the following sequence could occur:

  1. Thread 1 enters the set_environ context manager, sets old_value = None, os.environ['no_proxy'] = 'xyz'.
  2. Thread 2 enters set_environ, sets old_value = 'xyz', `os.environ['no_proxy'] = 'xyz'.
  3. Thread 1 exits, deletes os.environ['no_proxy'].
  4. Thread 1 exits, sets os.environ['no_proxy'] = 'xyz'.

Now after all the requests have been finished, the environment has been modified.

Reproduction Steps

I think this code can in theory trigger the race condition, although I haven't observed it. It needs an HTTP server on localhost:8080 (or just edit the URL)

#!/usr/bin/env python3

import threading
import os

import requests

def my_thread():
    session = requests.Session()
    for i in range(100):
        with session.get('http://localhost:8080/', proxies={'no_proxy': 'xyz'}) as resp:
            pass

threads = [threading.Thread(target=my_thread) for _ in range(16)]
for thread in threads:
    thread.start()
for thread in threads:
    thread.join()
print('no_proxy:', os.environ.get('no_proxy', 'not set'))

System Information

$ python -m requests.help
{
  "chardet": {
    "version": "3.0.4"
  },
  "cryptography": {
    "version": ""
  },
  "idna": {
    "version": "2.7"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.6.6"
  },
  "platform": {
    "release": "4.15.0-45-generic",
    "system": "Linux"
  },
  "pyOpenSSL": {
    "openssl_version": "",
    "version": null
  },
  "requests": {
    "version": "2.21.0"
  },
  "system_ssl": {
    "version": "1010007f"
  },
  "urllib3": {
    "version": "1.24.1"
  },
  "using_pyopenssl": false
}

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at requests.utils.should_bypass_proxies and the set_environ context manager named in the report; inspect how the temporary no_proxy environment change surrounds the httplib call. Run the supplied multithreaded reproduction, then verify that requests leaves the process environment unchanged after concurrent proxy handling.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.