Fix race in `FileResponse` if file is replaced during `prepare` #10101

bdraco · 2024-12-04T16:50:32Z

There was a race in FileResponse where the stat would be incorrect if the file was changed out between the stat and open syscalls. This would lead to various unexpected behaviors such as trying to read beyond the length of the file or sending a partial file. This problem is likely to occour when files are being renamed/linked into place.

An example of how this can happen with a system that provides weather data every 60s:

An external process writes .weather.txt at the top of each minute, and than renames it into place as weather.txt. In this case FileResponse.prepare may stat the old weather.txt, and than open the new weather.txt, and use the os.stat_result from the original file.

To fix this we now fstat the open file on operating systems where fstat is available.

This change looks much larger than it actually is because most of the function now needs to be wrapped in the try/finally to ensure that we close the file cleanly as we open it a lot sooner now. Its best to compare without white space as well.

fixes #8013

…d open calls There was a race in ``FileResponse`` where the stat would be incorrect if the file was changed out between the `stat` and `open` syscalls. This would lead to various unexpected behaviors such as trying to read beyond the length of the file or sending a partial file. This problem is likely to occour when files are being renamed/linked into place. An example of how this can happen with a system that provides weather data every 60s: An external process writes `.weather.txt` at the top of each minute, and than renames it to `weather.txt`. In this case `aiohttp` may stat the old `weather.txt`, and than open the new `weather.txt`, and use the `stat` result from the original file. To fix this we now `fstat` the open file on operating systems where `fstat` is available fixes #8013

codecov · 2024-12-04T16:53:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.75%. Comparing base (7557b03) to head (695e78c).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10101      +/-   ##
==========================================
- Coverage   98.76%   98.75%   -0.01%     
==========================================
  Files         122      122              
  Lines       36912    36927      +15     
  Branches     4402     4409       +7     
==========================================
+ Hits        36455    36469      +14     
  Misses        311      311              
- Partials      146      147       +1

Flag	Coverage Δ
CI-GHA	`98.64% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`98.33% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`96.17% <100.00%> (-0.01%)`	⬇️
OS-macOS	`97.44% <100.00%> (+0.01%)`	⬆️
Py-3.10.11	`97.28% <100.00%> (+<0.01%)`	⬆️
Py-3.10.15	`97.81% <100.00%> (-0.01%)`	⬇️
Py-3.11.10	`97.86% <100.00%> (-0.01%)`	⬇️
Py-3.11.9	`97.32% <100.00%> (-0.01%)`	⬇️
Py-3.12.7	`98.38% <100.00%> (-0.01%)`	⬇️
Py-3.13.0	`97.19% <100.00%> (-1.19%)`	⬇️
Py-3.13.1	`98.35% <100.00%> (?)`
Py-3.9.13	`97.19% <93.93%> (-0.01%)`	⬇️
Py-3.9.20	`97.73% <93.93%> (-0.01%)`	⬇️
Py-pypy7.3.16	`97.34% <93.93%> (-0.01%)`	⬇️
VM-macos	`97.44% <100.00%> (+0.01%)`	⬆️
VM-ubuntu	`98.33% <100.00%> (+<0.01%)`	⬆️
VM-windows	`96.17% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codspeed-hq · 2024-12-04T16:56:01Z

CodSpeed Performance Report

Merging #10101 will improve performances by 15.95%

_{Comparing race_wrong_stat (695e78c) with master (7557b03)}

Summary

⚡ 2 improvements
✅ 44 untouched benchmarks

Benchmarks breakdown

	Benchmark	`master`	`race_wrong_stat`	Change
⚡	`test_simple_web_file_response[pyloop]`	95.5 ms	82.3 ms	+15.95%
⚡	`test_simple_web_file_sendfile_fallback_response[pyloop]`	100.7 ms	88.8 ms	+13.43%

aiohttp/web_fileresponse.py

CHANGES/10101.bugfix.rst

bdraco · 2024-12-04T19:16:12Z

close reopen because github flaked in the middle of the CI run.

https://www.githubstatus.com/

Hopefully I don't have to make a new PR

patchback · 2024-12-04T20:13:09Z

Backport to 3.11: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.11/678993a4bd071200fbcc14fa121c6673221aa540/pr-10101

Backported as #10105

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

(cherry picked from commit 678993a)

patchback · 2024-12-04T20:13:17Z

Backport to 3.12: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.12/678993a4bd071200fbcc14fa121c6673221aa540/pr-10101

Backported as #10106

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

(cherry picked from commit 678993a)

aiohttp/web_fileresponse.py

…e is replaced during `prepare` (#10105) Co-authored-by: J. Nick Koston <[email protected]> fixes #8013

…e is replaced during `prepare` (#10106) Co-authored-by: J. Nick Koston <[email protected]> fixes #8013

Dreamsorcerer · 2024-12-07T00:26:26Z

@bdraco As far as I can see from codecov history, this PR is the one that loses coverage on web_response.py. Should we remove that check or adjust the tests to get coverage again?

https://app.codecov.io/gh/aio-libs/aiohttp/pull/10101/indirect-changes

bdraco · 2024-12-07T01:10:35Z

After fighting with

@bdraco As far as I can see from codecov history, this PR is the one that loses coverage on web_response.py. Should we remove that check or adjust the tests to get coverage again?

app.codecov.io/gh/aio-libs/aiohttp/pull/10101/indirect-changes

That's a bit surprising. I'm a bit curious what we were passing in there before that would have hit that branch

https://app.codecov.io/gh/aio-libs/aiohttp/pull/10095/blob/aiohttp/web_response.py#L268
https://app.codecov.io/gh/aio-libs/aiohttp/pull/10101/blob/aiohttp/web_response.py#L268

bdraco · 2024-12-07T01:14:43Z

That's a bit surprising. I'm a bit curious what we were passing in there before that would have hit that branch

@Dreamsorcerer
Checked out fcce1bf

Made a small change

diff --git a/aiohttp/web_response.py b/aiohttp/web_response.py
index cb3e3717c..907f061f5 100644
--- a/aiohttp/web_response.py
+++ b/aiohttp/web_response.py
@@ -267,6 +267,10 @@ class StreamResponse(BaseClass, HeadersMixin, CookieMixin):
             )
         elif isinstance(value, str):
             self._headers[hdrs.LAST_MODIFIED] = value
+        else:
+            raise ValueError(
+                "Unsupported type for last_modified: %s" % type(value).__name__
+            )
 
     @property
     def etag(self) -> Optional[ETag]:

And... it looks like it was only covering the branch because it was passing in a MagicMock before

FAILED tests/test_web_sendfile.py::test_gzip_if_header_not_present_and_file_not_available[pyloop] - ValueError: Unsupported type for last_modified: MagicMock
FAILED tests/test_web_sendfile.py::test_using_gzip_if_header_present_and_file_available[pyloop] - ValueError: Unsupported type for last_modified: MagicMock
FAILED tests/test_web_sendfile.py::test_gzip_if_header_present_and_file_not_available[pyloop] - ValueError: Unsupported type for last_modified: MagicMock
FAILED tests/test_web_sendfile.py::test_status_controlled_by_user[pyloop] - ValueError: Unsupported type for last_modified: MagicMock
FAILED tests/test_web_sendfile.py::test_gzip_if_header_not_present_and_file_available[pyloop] - ValueError: Unsupported type for last_modified: MagicMock

bdraco · 2024-12-07T04:44:29Z

Maybe raising ValueError when it's going to do nothing does make the most sense (along with a test for it). However not sure if we should backport something like that to 3.x as the current behavior is to swallow unsupported types and raising an exception might be breaking change.

bdraco · 2024-12-07T14:37:13Z

Captured as #10143

bdraco added backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot labels Dec 4, 2024

bdraco added 5 commits December 4, 2024 10:56

lint

e79d9c6

back compat

4b0cd14

lint

74b9390

comments

6259681

comments

2a41636

github-advanced-security bot found potential problems Dec 4, 2024

View reviewed changes

aiohttp/web_fileresponse.py Fixed Show fixed Hide fixed

bdraco added 6 commits December 4, 2024 11:40

no way to avoid double stat

053984a

reverts

bdef0a9

lint

9664b5e

remove unreachable code

b376555

comments

caeac0f

preen

2efe67f

bdraco changed the title ~~Fix race in FileResponse if file is replaced during prepare~~ Fix race in FileResponse if file is replaced during prepare Dec 4, 2024

changelog

0f7362b

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 4, 2024

bdraco commented Dec 4, 2024

View reviewed changes

CHANGES/10101.bugfix.rst Outdated Show resolved Hide resolved

Update CHANGES/10101.bugfix.rst

367dda5

bdraco closed this Dec 4, 2024

bdraco reopened this Dec 4, 2024

bdraco commented Dec 4, 2024

View reviewed changes

CHANGES/10101.bugfix.rst Outdated Show resolved Hide resolved

Update CHANGES/10101.bugfix.rst

52d7719

bdraco closed this Dec 4, 2024

bdraco reopened this Dec 4, 2024

bdraco commented Dec 4, 2024

View reviewed changes

CHANGES/10101.bugfix.rst Outdated Show resolved Hide resolved

Update CHANGES/10101.bugfix.rst

db087f2

bdraco marked this pull request as ready for review December 4, 2024 19:10

bdraco requested review from webknjaz and asvetlov as code owners December 4, 2024 19:10

bdraco closed this Dec 4, 2024

bdraco reopened this Dec 4, 2024

Merge branch 'master' into race_wrong_stat

695e78c

bdraco merged commit 678993a into master Dec 4, 2024
40 checks passed

bdraco deleted the race_wrong_stat branch December 4, 2024 20:13

patchback bot pushed a commit that referenced this pull request Dec 4, 2024

Fix race in FileResponse if file is replaced during prepare (#10101)

fe03729

(cherry picked from commit 678993a)

patchback bot mentioned this pull request Dec 4, 2024

[PR #10101/678993a4 backport][3.11] Fix race in FileResponse if file is replaced during prepare #10105

Merged

patchback bot pushed a commit that referenced this pull request Dec 4, 2024

Fix race in FileResponse if file is replaced during prepare (#10101)

1bd4cf1

(cherry picked from commit 678993a)

patchback bot mentioned this pull request Dec 4, 2024

[PR #10101/678993a4 backport][3.12] Fix race in FileResponse if file is replaced during prepare #10106

Merged

steverep reviewed Dec 4, 2024

View reviewed changes

aiohttp/web_fileresponse.py Show resolved Hide resolved

aiohttp/web_fileresponse.py Show resolved Hide resolved

bdraco pushed a commit that referenced this pull request Dec 4, 2024

[PR #10101/678993a4 backport][3.11] Fix race in FileResponse if fil…

07d1759

…e is replaced during `prepare` (#10105) Co-authored-by: J. Nick Koston <[email protected]> fixes #8013

bdraco pushed a commit that referenced this pull request Dec 4, 2024

[PR #10101/678993a4 backport][3.12] Fix race in FileResponse if fil…

fa6804c

…e is replaced during `prepare` (#10106) Co-authored-by: J. Nick Koston <[email protected]> fixes #8013

steverep mentioned this pull request Dec 4, 2024

FileResponse has undefined behavior if the file is changed out from under it between the stat and open calls #8013

Closed

1 task

This was referenced Dec 5, 2024

Restore performance of 304s after #10101 #10112

Closed

Restore 304 performance after fixing FileResponse replace race #10113

Merged

This was referenced Dec 5, 2024

[PR #10113/01302134 backport][3.11] Restore 304 performance after fixing FileResponse replace race #10117

Merged

[PR #10113/01302134 backport][3.12] Restore 304 performance after fixing FileResponse replace race #10118

Merged

bdraco mentioned this pull request Dec 5, 2024

Typing improvements for file responses #10122

Merged

bdraco mentioned this pull request Dec 7, 2024

Raise TypeError when last_modified gets an unexpected type #10143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race in `FileResponse` if file is replaced during `prepare` #10101

Fix race in `FileResponse` if file is replaced during `prepare` #10101

bdraco commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

codspeed-hq bot commented Dec 4, 2024 •

edited

Loading

bdraco commented Dec 4, 2024

patchback bot commented Dec 4, 2024 •

edited

Loading

patchback bot commented Dec 4, 2024 •

edited

Loading

Dreamsorcerer commented Dec 7, 2024

bdraco commented Dec 7, 2024

bdraco commented Dec 7, 2024

bdraco commented Dec 7, 2024

bdraco commented Dec 7, 2024

Fix race in FileResponse if file is replaced during prepare #10101

Fix race in FileResponse if file is replaced during prepare #10101

Conversation

bdraco commented Dec 4, 2024 • edited Loading

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Dec 4, 2024 • edited Loading

CodSpeed Performance Report

Merging #10101 will improve performances by 15.95%

Summary

Benchmarks breakdown

bdraco commented Dec 4, 2024

patchback bot commented Dec 4, 2024 • edited Loading

Backport to 3.11: 💚 backport PR created

patchback bot commented Dec 4, 2024 • edited Loading

Backport to 3.12: 💚 backport PR created

Dreamsorcerer commented Dec 7, 2024

bdraco commented Dec 7, 2024

bdraco commented Dec 7, 2024

bdraco commented Dec 7, 2024

bdraco commented Dec 7, 2024

Fix race in `FileResponse` if file is replaced during `prepare` #10101

Fix race in `FileResponse` if file is replaced during `prepare` #10101

bdraco commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

codspeed-hq bot commented Dec 4, 2024 •

edited

Loading

patchback bot commented Dec 4, 2024 •

edited

Loading

patchback bot commented Dec 4, 2024 •

edited

Loading