Skip to content

gh-150107: adding hasattr check on offset to include if available#150142

Open
grantlouisherman wants to merge 14 commits into
python:mainfrom
grantlouisherman:bugfix-150107-asyncio-ignore-0-offset
Open

gh-150107: adding hasattr check on offset to include if available#150142
grantlouisherman wants to merge 14 commits into
python:mainfrom
grantlouisherman:bugfix-150107-asyncio-ignore-0-offset

Conversation

@grantlouisherman
Copy link
Copy Markdown

@grantlouisherman grantlouisherman commented May 20, 2026

Original Bug Report:

Bug report

Bug description:

BaseEventLoop._sock_sendfile_fallback (https://github.com/python/cpython/blob/main/Lib/asyncio/base_events.py#L971) erroneously doesn't seek when passed the offset 0.

        if offset:
            file.seek(offset)

This check in the function doesn't call file.seek if offset is falsey. If the file in question has a file pointer that is not currently at the 0 offset then this will erroneously send from the current offset and not the start of the file as desired. This can happen when sendfile is called with the destination socket is an SSL socket.

Repro code (generated by Claude), because of needing to create an SSL socket it's a bit involved - including making a self signed cert with the openssl command.

import asyncio
import os
import ssl
import subprocess
import sys
import tempfile
from pathlib import Path

CONTENT = b"X" * 1000


def make_throwaway_cert(tmp: Path) -> tuple[Path, Path]:
    cert = tmp / "cert.pem"
    key = tmp / "key.pem"
    subprocess.run(
        [
            "openssl", "req", "-x509", "-newkey", "rsa:2048", "-nodes",
            "-keyout", str(key), "-out", str(cert),
            "-days", "1", "-subj", "/CN=localhost",
        ],
        check=True,
        capture_output=True,
    )
    return cert, key


async def main() -> int:
    with tempfile.TemporaryDirectory() as tmp_:
        tmp = Path(tmp_)
        cert, key = make_throwaway_cert(tmp)

        data_path = tmp / "data"
        data_path.write_bytes(CONTENT)

        # SSL server that drains whatever it receives.
        server_ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
        server_ctx.load_cert_chain(cert, key)
        received = 0
        done = asyncio.Event()

        async def handle(reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> None:
            nonlocal received
            try:
                while chunk := await reader.read(4096):
                    received += len(chunk)
            finally:
                writer.close()
                done.set()

        server = await asyncio.start_server(handle, "127.0.0.1", 0, ssl=server_ctx)
        port = server.sockets[0].getsockname()[1]

        # SSL client → transport's `_sendfile_compatible == UNSUPPORTED` → fallback path.
        client_ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
        client_ctx.check_hostname = False
        client_ctx.verify_mode = ssl.CERT_NONE
        _, writer = await asyncio.open_connection(
            "127.0.0.1", port, ssl=client_ctx, server_hostname="localhost"
        )

        # Open the file and pre-advance its position. In the real-world bug
        # this happens because the calling code previously read through the
        # same fh (e.g. to parse an HTTP header).
        f = open(data_path, "rb", buffering=0)
        f.seek(len(CONTENT))  # file position is at EOF

        # Send from offset=0. Expectation: 1000 bytes transferred.
        loop = asyncio.get_running_loop()
        sent = await loop.sendfile(writer.transport, f, offset=0)

        writer.close()
        await writer.wait_closed()
        await done.wait()
        server.close()
        await server.wait_closed()
        f.close()

        print(f"Python:       {sys.version}")
        print(f"file size:    {len(CONTENT)}")
        print(f"file.tell() before sendfile: {len(CONTENT)} (EOF)")
        print(f"loop.sendfile(..., offset=0) returned: {sent}")
        print(f"server received: {received} bytes")
        print()

        if sent == 0 and received == 0:
            print("BUG REPRODUCED: offset=0 was ignored; the fallback read from "
                  "file.tell() instead of from the requested offset.")
            return 1
        else:
            print("Bug NOT reproduced on this Python build.")
            return 0


sys.exit(asyncio.run(main()))

The relevant lines are the line f.seek(len(CONTENT)) and the line sent = await loop.sendfile(writer.transport, f, offset=0). Most of the rest is setup and teardown.

CPython versions tested on:

3.12, 3.13

Operating systems tested on:

Linux

Fix

I think what I did was correct or understood the issue, but essentially as far as I could understand 0 is always falsey so if offset was ever set to zero than the file.seek wasent happening even though 0 offset is a legitimate offset. I simply just did an instance on offset to make sure it is an int and then the f.seek goes through. I validated with the repro command and passing unit tests.

…able

Signed-off-by: grantlouisherman <grantlouisherman041@gmail.com>
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 20, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@grantlouisherman grantlouisherman changed the title gh150107: adding isinstance check on offset to include if available gh-150107: adding isinstance check on offset to include if available May 20, 2026
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 20, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@grantlouisherman
Copy link
Copy Markdown
Author

Hey @1st1 does this need a news blurb? IT seems pretty small

@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 20, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@grantlouisherman
Copy link
Copy Markdown
Author

Also I cant add the tags but this needs to be back ported to 3.12 and 3.13

@picnixz
Copy link
Copy Markdown
Member

picnixz commented May 21, 2026

  1. Bugfixes always need NEWS entries.
  2. Backport labels are added by maintainers and triagers.
  3. 3.12 is security-only and will not be fixed unless this is a security issue

@grantlouisherman
Copy link
Copy Markdown
Author

Sounds good @picnixz Ill add news now

@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 21, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@grantlouisherman grantlouisherman changed the title gh-150107: adding isinstance check on offset to include if available gh-150142: adding isinstance check on offset to include if available May 21, 2026
@grantlouisherman grantlouisherman changed the title gh-150142: adding isinstance check on offset to include if available gh-150107: adding isinstance check on offset to include if available May 21, 2026
Comment thread Lib/asyncio/base_events.py
Comment thread Misc/NEWS.d/next/Core_and_Builtins/2026-05-21-14-17-29.gh-issue-150107.hwPKW6.rst Outdated
Signed-off-by: grantlouisherman <grantlouisherman041@gmail.com>
Signed-off-by: grantlouisherman <grantlouisherman041@gmail.com>
@grantlouisherman grantlouisherman changed the title gh-150107: adding isinstance check on offset to include if available gh-150107: adding hasattr check on offset to include if available May 21, 2026
Signed-off-by: grantlouisherman <grantlouisherman041@gmail.com>
@vstinner
Copy link
Copy Markdown
Member

You can add the following sock_sendfile() tests to Lib/test/test_asyncio/test_sendfile.py after test_sock_sendfile_zero_size():

    def check_sock_sendfile_offset(self, data, offset, force_fallback=False):
        sock, proto = self.prepare_socksendfile()
        with tempfile.TemporaryFile() as f:
            f.write(data)
            f.flush()
            self.assertEqual(f.tell(), len(data))

            if force_fallback:
                async def _sock_sendfile_fail(sock, file, offset, count):
                    raise asyncio.exceptions.SendfileNotAvailableError()
                with support.swap_attr(self.loop, '_sock_sendfile_native', _sock_sendfile_fail):
                    ret = self.run_loop(self.loop.sock_sendfile(sock, f, offset, None))
            else:
                ret = self.run_loop(self.loop.sock_sendfile(sock, f, offset, None))

            self.assertEqual(f.tell(), len(data))

        sock.close()
        self.run_loop(proto.wait_closed())

        self.assertEqual(ret, len(data) - offset)

    def test_sock_sendfile_offset(self):
        data = b'abcdef'
        for offset in (0, len(data) // 2, len(data)):
            for force_fallback in (False, True):
                with self.subTest(offset=offset, force_fallback=force_fallback):
                    self.check_sock_sendfile_offset(data, offset, force_fallback)

@vstinner
Copy link
Copy Markdown
Member

I also wrote a draft test for sendfile(), but the check on self.file.tell() fails for an unknown reason, so I commented the check below:

    def check_sendfile_offset(self, offset, fallback):
        srv_proto, cli_proto = self.prepare_sendfile()
        self.file.seek(123)
        coro = self.loop.sendfile(cli_proto.transport, self.file, offset, fallback=fallback)
        ret = self.run_loop(coro)
        cli_proto.transport.close()
        self.run_loop(srv_proto.done)
        self.assertEqual(ret, len(self.DATA) - offset)
        self.assertEqual(srv_proto.nbytes, len(self.DATA) - offset)
        self.assertEqual(srv_proto.data, self.DATA[offset:])
        #self.assertEqual(self.file.tell(), len(self.DATA))

    def test_sendfile_offset(self):
        for offset in (0, len(self.DATA) // 2, len(self.DATA)):
            for fallback in (False, True):
                with self.subTest(offset=offset, fallback=fallback):
                    self.check_sendfile_offset(offset, fallback)

Signed-off-by: grantlouisherman <grantlouisherman041@gmail.com>
@grantlouisherman
Copy link
Copy Markdown
Author

grantlouisherman commented May 21, 2026

@vstinner I just c/p your tests, I was going to try and use whatever the original repro was but I wasnt sure how to make that into test form so thanks for the snippet~

Comment thread Lib/test/test_asyncio/test_sendfile.py
Comment thread Lib/test/test_asyncio/test_sendfile.py
Comment thread Lib/test/test_asyncio/test_sendfile.py
Signed-off-by: grantlouisherman <grantlouisherman041@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants