-
Notifications
You must be signed in to change notification settings - Fork 581
open() on ref to many MBs long scalar, then <$fh>, causes fatal OOM in pp_readline and sv_gets, probably Win32-only #22623
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
Comments
It's part of #21877 and not Windows specific, though it appears to cause worse performance on Windows than on Linux (probably due to Linux not committing all allocations by default) |
#21654 related copy pasting relevent parts of #5454 (comment) below
NOTICE PERL's
This is still a problem in 5.41.8/Win64/Win7. IDK if MS ever improved this in Win10/11. |
|
re: malloc performance: I'd noticed when doing some profiling that the ucrt malloc was using a remarkably low amount of processing time (the error number saving, which requires GetLastError()/SetLastError() was significant) of a perl allocation call. Unfortunately the wrappers add cost. large realloc() performance: unfortunately fixing this would require that NT implemented something like mremap(), I suspect the NT memory management model doesn't allow you to remap part of a memory mapping, while on Linux the following succeeds:
|
Correct, I single stepped my Win 7 HeapAlloc() in LFH mode (OS default since >= Vista), a totally random <=64 bytes callsite I used for this instruction count,
I have a PR 80% finished turning WinPerl's
I think its sort of possible to do on NT but multiple roadblocks exist. First one is, once a NT mmap "view"/"address space" refcounted kernel object is created, its start pointer and end pointer can never be changed for the rest of the life of that "address space" object. To "grow" it, there are 2 choices, make a 65KB long 2nd address space adjacent "address space" refcounted kernel object, or create a 2nd longer in bytes "address space" refcounted kernel object at a random new address, and include the "virtual unmapped pages" from previous address space object's backing storage object as the new bottom half of the new "address space" kernel object. "backing storage object" is obviously either the official Windows paging file, or another temp file on the disk, possibly being an NTFS anonymous/or NTFS delete_pending flagged file (think unix). 2nd roadblock is, "address space" refcounted kernel objects come in contiguous units of 65KB and are aligned at 65KB in addr space. The 4KB pages inside a unit of 65KB can be manipulated (commit->reserved->ro->cow->rw commit->reserved etc) individually at runtime, but using only the first 4KB page of a 65KB object, and keeping the rest marked as reserved/no access, will blow out 2GB/3.5GB of i386 address space pretty quick, and cause a system wide GUI near-lockup/ultra-slow GUI on x64, and 100% CPU on 1 core in "System process". I know Linux mmap allows units of 4KB, of a random length of units, aligned at 4KB, as address space objects. NT kernel only allow 65KB units, but you can still flip the MMU bits individually on the 4KB units that make up the 65KB units. Basically MS decided in 1980s/1993, for performance reasons, they will not "hash table track" the 1st page table layer, on i386, which is 1024 long == 2^10 or 2^12 if I read this https://wiki.osdev.org/Paging. Or another quick theory, 2^4=16, 4KB * 16 == 65 KB, and 2^12, takes 12 bits away, leaving bits 13, 14, 15, 16 unused inside a U16. So WinNT picked data locality/least mem pointer derefs as a design goal during a page fault. Unix/POSIX spec/Linux picked developer friendliness instead of performance as a design goal. Probably Raymond Chen has gossip on the design choice, but it the reasons/rational are irrelevant, Its not changing any time soon. For Perl 5's purposes, if WinPerl wants to do massive grow/shrink cycles in deltas/units of 100s of KBs/MBs at a time, the question is why. 2 ways this can go I think Either there are serious algorithmic bugs of why the buffer was so over-estimated or over-extended originally and those algo bugs are bugs, not optimizations, as originally described by their authors. Or conceptual SvCUR() used to be that big but naturally got smaller, and now that "tail" is wasted and a significant (no criteria) amount of waste and needs to be GCed back to MS CRT/HeapAlloc/p5p's malloc.c/AnyOSLibC/WinPerl's CPHost malloc. If you will grep the MS CRT source code, you might find a non-sequitor src code comment, that describes a MS internal/private CI test that "verifies" the MS CRT will not SEGV/hang/deadlock, with exactly 2^32-8096 or 2^32-(4096+2048) free malloc()/HeapAlloc() bytes left in the process, and that MS has a "contractual obligation" with one of their very large customers to run safely/stable under that extreme environment. Ive wondered about that comment in the UCRT for a while, I can only guess that means, the PC has run out of disk space and therefore paging file space, so NtAllocateVirtualMemory() fails, but 8KB isn't 4KB or 65KB (NT kernel MMU object size). I recently thought of something else is going on...... Which app developer/company, decided that executing That ^^^^ design is improper IMO for P5 interp to be doing. If the PP VM/C or XS VM state really really needs to "shrink" a malloc block on WinPerl, WinPerl needs special casing to malloc() a new smaller block and copy over the data, and call the free() on the old block. Not using the UB from POSIX through
Recent update, Seems like this HeapReAlloc()/CRT realloc() "refuse to go backwards"/"shrink in place" like Unix malloc() bug is MUCH more wide spread inside the interp. I profiles And it seems to me on my Win7 OS, for 16-128 byte long blocks, Ofc it does a Here 1 problematic backtrace, I included more in an attachment, all of them are regexp engine related. regexp_engine_mktables_realloc_cpu_burn_call_stacks.txt
|
open()
on a large scalar (MBs worth), with LF lines, NOT CRLF lines, then calling<$fh>
aka pp_readline(), causes a mem leak and a fatal "Out of memory!" on 5.32. 5.41 error is similar but more verbose. Tested both 5.41.5 32b and strawberry-perl 5.32.1.1 32b. Both fatal OOM.Call stack at realloc() fail return NULL and with 5.41.5 blead perl. Line numbers are fuzzy since this is a -O1/release perl build.
The toxic sv_grow() is called from
The 14MB realloc count I THINK comes from
since the open() on $scalar, the PerlIO backend is just returning the whole scalar's len. I'm not sure that readline() having an infinity buffer. is the best design, since in my case, for a private biz app, I was doing open() on $scalar, and $scalar was a RO mmap string from File::Map. The bug does not involve File::Map. My repro script doesn't use File::Map. The bug is more specifically the loops in readline() or sv_gets() .
I suspect the SvPV_shrink_to_cur(sv); isn't really working on Win32 or there is a perl level leak until next FREETMPS/NEXTSTATE, in the 2 loops (in pp_readline() and sv_gets()). Note in the attached screenshots, the 14 MB alloc over and over. Im not able to fully debug this, but either the Perl SVPV shrink is broken, and the block isn't being shrunk by malloc()/HeapAlloc/RtlHeapWhatever, and stays at the full 14MB for the rest of the lifetime of the malloc block. Or the 2 loops are actually leaking.
Note Win32 realloc/HeapReAlloc MIGHT BE totally incapable of shrinking buffers (an "optimization"), or HeapReAlloc adds the "14MB-80??? bytes" "released" "free-ed 4096 byte memory pages" to the <= 16KB/64KB/128KB/512KB user mode alloc pools ( https://www.blackhat.com/docs/us-16/materials/us-16-Yason-Windows-10-Segment-Heap-Internals-wp.pdf ). But Perl keeps asking for "14MB" chunks, and Win32 Heap by design/policy/API/ABI (oh no on ABI) back compat, all 14MB allocs must be raw rounded up 4096 pages VM and must come from the kernel raw VM page allocator. So maybe "fragmentation" or the 512KB rule causes OOM on 32b perl. I didn't test the repro on 64b perl.
To repeat, IDK if this is a Perl level leak until FREETMPS or a Win32 Heap API problem, I have 3 hypothesis tho described above. In any case its an OOM and a pretty bad bug, since it is rooted in CRLF/LF and pretty simple to accidentally trigger, but someone could argue that open() on a scalar is very rare and perf degrad/poor design, since you might as well use index() and substr() for perf if the whole file is already in a scalar string.
I did observe the realloc() len arg, was slowly dropping 1 or 100 ish or 500 ish bytes per loop iteration. Screenshot shows the VM OS alloc eventually starts dropping in 4KB pages which confirms the pp_readline() or sv_gets() leak is O(n^2) ish but slowly drops.
Im leaving this crash to someone else, there are too many optimizations and tricks in pp_readline and sv_gets, and I didn't spot an obv quick fix to the leak. Plus my quick glance left more design questions, then answers, because of all the lvalue PV buffer swapping/save stacking/IDK what optimizations in there. I thought/hypo-ed that changing pp_readline() to a 80 byte or 256 or 1024ish byte buffer limit might superficially stop the OOM, but still be leaving leaks until NEXTSTATE/FREETMPS in the code, so I dont see a instant simple fix. Plus design debate, about malloc/memcpying, the ENTIRE!!! PerlIO open( )on $scalar's backend SVPV, from rvalue to lvalue, Even if src backend SVPV is 100's of MBs long!!!! hence this tkt.
Steps to Reproduce
Note the 32b perl, the 16MB input file len , and the LF input file, and binmode :raw. Since I didnt test this on Linux, this might be a Win32 only Perl bug and not repro on Linux, since Perl default record sep must be CRLF, and my input is LF. I didn't test the crash script with a 16MB CRLF input .csv. The "14MB" input I mention, was the original private CSV file that lead to this bug. The 16MB .csv in the repro script same style input.
5.32 fatal OOM output
Expected behavior
<$fh>
returns separated string lines, or returns undef. Not fatal OOM.Perl configuration
Also OOM fails on 32b 5.41.5.
The text was updated successfully, but these errors were encountered: