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

test_os.test_memfd_create() fails on AMD64 FreeBSD Shared 3.x #85185

Closed
vstinner opened this issue Jun 17, 2020 · 16 comments
Closed

test_os.test_memfd_create() fails on AMD64 FreeBSD Shared 3.x #85185

vstinner opened this issue Jun 17, 2020 · 16 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes deferred-blocker tests Tests in the Lib/test dir

Comments

@vstinner
Copy link
Member

BPO 41013
Nosy @vstinner, @tiran, @ambv, @koobs, @kevans91

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-01-29.21:50:18.919>
created_at = <Date 2020-06-17.22:52:12.731>
labels = ['3.8', 'deferred-blocker', 'tests', '3.9', '3.10']
title = 'test_os.test_memfd_create() fails on AMD64 FreeBSD Shared 3.x'
updated_at = <Date 2021-01-29.21:50:18.919>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2021-01-29.21:50:18.919>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-01-29.21:50:18.919>
closer = 'vstinner'
components = ['Tests']
creation = <Date 2020-06-17.22:52:12.731>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 41013
keywords = []
message_count = 16.0
messages = ['371780', '371792', '371799', '371810', '371811', '371814', '371815', '371818', '372337', '372678', '372682', '373597', '373598', '374457', '374463', '385947']
nosy_count = 5.0
nosy_names = ['vstinner', 'christian.heimes', 'lukasz.langa', 'koobs', 'kevans']
pr_nums = []
priority = 'deferred blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue41013'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

@vstinner
Copy link
Member Author

https://buildbot.python.org/all/#/builders/152/builds/1024
...
test_makedir (test.test_os.MakedirTests) ... ok
test_mode (test.test_os.MakedirTests) ... ok
Timeout (0:25:00)!
Thread 0x0000000800b54000 (most recent call first):
File "/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Lib/test/test_os.py", line 3520 in test_memfd_create
...

@vstinner vstinner added 3.10 only security fixes tests Tests in the Lib/test dir labels Jun 17, 2020
@tiran
Copy link
Member

tiran commented Jun 18, 2020

Commit bb6ec14 fixed a problem with tests, so the test is now executed. Does FreeBSD have an implementation of memfd_create that behaves slightly differently than memfd_create on Linux?

@koobs
Copy link

koobs commented Jun 18, 2020

memfd_create came in via:

freebsd/freebsd-src@575e351

via review: https://reviews.freebsd.org/D21393

via kevans (cc'd)

@kevans91
Copy link
Mannequin

kevans91 mannequin commented Jun 18, 2020

Interesting; I don't quite have time to look at the moment, but what is the test doing that it's ultimately timing out?

Our memfd_create is assumed to be compatible, with the exception that we don't yet support HUGETLB (but there are patches in progress for the vm that might make it feasible); in my experience most users of memfd_create weren't really using hugetlb, though.

@tiran
Copy link
Member

tiran commented Jun 18, 2020

The traceback points to line 3520, which calls f.tell() on a file object that wraps a memfd.

@kevans91
Copy link
Mannequin

kevans91 mannequin commented Jun 18, 2020

I think it's too early to look at this, I'll circle back another time. :-) I got a minute and extracted the relevant test, but I guess there must have been some fundamental transcription error:

import os

fd = os.memfd_create("Hi", os.MFD_CLOEXEC)
if fd == -1:
    print("boo")
    os.exit(1)

if os.get_inheritable(fd):
    print("inheritable, boo")
    os.exit(1)

with open(fd, "wb", closefd=False) as f:
    f.write(b'memfd_create')
    if f.tell() != 12:
        print("Hork")
        os.exit(1)
    print("What?")

print("Done")

When I run this with python3.9, I get:

...
3038 100554: shm_open2(SHM_ANON,O_RDWR|O_CLOEXEC,00,0,"memfd:Hi") = 3 (0x3)
3038 100554: fcntl(3,F_GETFD,) = 1 (0x1)
3038 100554: fstat(3,{ mode=---------- ,inode=10,size=0,blksize=4096 }) = 0 (0x0)
3038 100554: ioctl(3,TIOCGETA,0x7fffffffe2a0) ERR#25 'Inappropriate ioctl for device'
3038 100554: lseek(3,0x0,SEEK_CUR) = 0 (0x0)
3038 100554: lseek(3,0x0,SEEK_CUR) = 0 (0x0)
3038 100554: write(3,"memfd_create",12) = 0 (0x0)
3038 100554: write(3,"memfd_create",12) = 0 (0x0)
3038 100554: write(3,"memfd_create",12) = 0 (0x0)
3038 100554: write(3,"memfd_create",12) = 0 (0x0)
3038 100554: write(3,"memfd_create",12) = 0 (0x0)
(ad infinitum)

So in my local repro, Python is looping forever on successful write() for reasons I'm not immediately sure of.

@kevans91
Copy link
Mannequin

kevans91 mannequin commented Jun 18, 2020

(Transcription error beyond the bogus os.exit() :-))

@kevans91
Copy link
Mannequin

kevans91 mannequin commented Jun 18, 2020

Ah, now that I'm more awake, I see the problem- the write is unsuccessful because I haven't yet implemented grow-on-write. None of the consumers that I had found relied on it, instead choosing to ftruncate() it to the size they need before operating on it.

I think the best course of action might be to disable the test on FreeBSD for now (unless you're ok with tossing in a truncate to expand the file), because it might take me a little bit to get around to this one.

@vstinner
Copy link
Member Author

So in my local repro, Python is looping forever on successful write() for reasons I'm not immediately sure of.

io.BufferedWriter.write() (well, especially its flush() method) calls write() until all data is written.

Extract of _bufferedwriter_flush_unlocked() code, Modules/_io/buffered.c:

    while (self->write_pos < self->write_end) {
        n = _bufferedwriter_raw_write(self,
            self->buffer + self->write_pos,
            Py_SAFE_DOWNCAST(self->write_end - self->write_pos,
                             Py_off_t, Py_ssize_t));
        if (n == -1) {
            goto error;
        }
        else if (n == -2) {
            _set_BlockingIOError("write could not complete without blocking",
                                 0);
            goto error;
        }
        self->write_pos += n;
        self->raw_pos = self->write_pos;
        written += Py_SAFE_DOWNCAST(n, Py_off_t, Py_ssize_t);
        /* Partial writes can return successfully when interrupted by a
           signal (see write(2)).  We must run signal handlers before
           blocking another time, possibly indefinitely. */
        if (PyErr_CheckSignals() < 0)
            goto error;
    }

You are correct: if write() returns 0, write() is called again. If write() always returns 0, the loop never stops...

Maybe a BlockingIOError must be raised if write(buffer) returns 0 (and buffer is not empty).

@ambv
Copy link
Contributor

ambv commented Jun 30, 2020

As a regression this would have been a release blocker, this also fails on 3.9 and 3.8. However, given that this has only been surfaced by Christian's fix to the testing machinery in #65141, I'll mark this as deferred blocker instead for visibility.

@ambv ambv added 3.8 only security fixes 3.9 only security fixes deferred-blocker labels Jun 30, 2020
@kevans91
Copy link
Mannequin

kevans91 mannequin commented Jun 30, 2020

Ah, sorry, I meant to update this- I submitted our fix for review a day or two ago, got approval for commit and will poke koobs to rebuild the FreeBSD -CURRENT buildbot with it after that.

@koobs
Copy link

koobs commented Jul 13, 2020

I've updated the FreeBSD CURRENT buildbot past base/r363065 [1] which implements SHM_GROW_ON_WRITE:

https://svnweb.freebsd.org/changeset/base/363065
https://reviews.freebsd.org/D25502

Also worth noting that I don't believe stable/12 (FreeBSD 12.x) will be getting this syscall (correct me if im wrong Kyle), so these tests may still fail on the FreeBSD 12.x worker, and the tests should be updated to account for that in some manner.

@kevans91
Copy link
Mannequin

kevans91 mannequin commented Jul 13, 2020

I can confirm that neither 12 nor 11 will be getting memfd_create; file sealing is a little more complicated to MFC, and I don't want to provide memfd_create in a place where it can't be paired with file sealing in case applications assume they come hand-in-hand and want to use sealing for freezable shm type stuff.

@kevans91
Copy link
Mannequin

kevans91 mannequin commented Jul 28, 2020

Hey koobs,

Can you confirm that this is fine now after I implemented SHM_GROW_ON_WRITE?

@koobs
Copy link

koobs commented Jul 28, 2020

@kyle Yes, msg373597 freebsd CURRENT buildbot passes, but freebsd 12/stable does not (cannot, since stable/12 wont get the syscall)

That means the Python memfd functions and tests need to account for this

@vstinner
Copy link
Member Author

test_os pass again on FreeBSD Shared 3.x, I close the issue.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes deferred-blocker tests Tests in the Lib/test dir
Projects
None yet
Development

No branches or pull requests

4 participants