Hi everyone, I wanted to follow up on the core review for my feature request. The last change happened on 7 July. I pinged the issue two weeks ago, as suggested by the pull request guidelines. Any updates or feedback would be greatly appreciated!
1 Like
Reviewed and merged, thanks! This had been on my radar as interesting but I hadnât had time.
Here is a clickable link to the PR for convenience: gh-121313: Limit the reading size from pipes to their default buffer size on Unix systems by aplaikner ¡ Pull Request #121315 ¡ python/cpython ¡ GitHub
2 Likes
Thank you! I have been working on a similar issue regarding Unix sockets, where basically the same drawbacks happen, just with a larger reading size. Iâll create an issue and pull a request to update the newly merged code with one additional line since my tests have shown that the 64 KiB limit (on systems with a base page size of 4 KiB) we set for pipes is also the best limit for Unix sockets.
1 Like
FYI, as added context, there has been some discussion around limiting large or read sizes elsewhere as well this year. Kicked off by @storchaka seeing it maybe a denial of service security concern where the read length comes from untrusted data and thus a huge allocation wastes virtual address space (which might trigger an OOM killer in some configurations?).
Iâm not yet convinced of the practical security importance of that (virtual vs dirtied address space being different concepts) but the theme is similar: âBetterâ buffer size choices on read system calls. With a consequence of more read system calls on large data (but different types of memory allocation calls as youâve noted in your changes) and sometimes more dirtied-page intermediate space allocation and memory copies. Itâs an annoying trade off to make decisions around.
1 Like
Thank you for this additional context, I wasnât aware of the security implications. Iâve created a PR for the aforementioned socket issue, limiting reads to the same size as pipes.
To comment on the problem of âwastingâ virtual address space. I think thatâs not the core problem, as you already mentioned, but rather the starting point for a landslide of other issues. Big VMAs let Linux install rather large huge pages, since the current policy is: Install the biggest transparent huge page possible, with regard to VMA size and alignment. Furthermore, a lot more management overhead in form of system calls is created when managing such large input buffers.
Limiting input buffer + reading sizes down to something like 64 KiB is a good solution, since it avoids both the creation of large transparent huge pages, and the management of huge VMAs, since the data chunk is small enough to the be put on the default heap. Here also a distinction needs to be made: Choosing a limit that results in the data being put on the default heap is not quite enough, since if the size if big enough, the heap top is shifted often using brk() syscalls. Sizes around 64-128 KiB behave well (on my system with a base page size of 4 KiB), while not increasing read() syscalls by a noticeable amount.
1 Like
I sympathize with the intent but is there a reliable application-agnostic heuristic to choose the buffer size? If the buffer size is too small and you need several read calls to satisfy the read size requested by the caller, then performance will decrease because of 1) issuing more system calls 2) incurring more reallocation costs.
I think in general the best place to choose a buffer size ceiling is the application or intermediate layer (that has a rough idea of the kind of data being read, which kind of file-like thing it is reading from, and therefore of typically reasonable buffer sizes), rather than Pythonâs OS abstraction layer where all that information is entirely lost.
Which also justifies the original PR in this discussion thread, because the multiprocessing module knows indeed the kind of data itâs reading and the kind of file descriptor itâs reading it from.
As for PR https://github.com/python/cpython/pull/119204, I think an alternative approach, instead of hardcoding arbitrary ceilings on read sizes, would be to allow passing a hard memory limit to the Unpickler such that any attempt to read a cumulated amount more than that would raise an exception (instead of allocating potentially unbounded read buffers).
For example in Thrift you can define a cumulated string size limit and a cumulated container size limit when deserializing a message:
Besides, in the common case where the pickle is deserialized from memory (pickle.loads), you already know the max byte size you can read from the pickle. In this case, you can easily bail out if any opcode tries to read more than whatâs remaining.
1 Like