regcomp_invlist.c -> S_invlist_trim -> SvPV_renew wasteful NOOP (bug demo, ! a PR)

[bulk88]

perl5/sv.h

Line 1574 in 7946d6a

STMT_START { SvLEN_set(sv, n); \

perl5/sv.h

Line 1593 in 7946d6a

#define SvPV_shrink_to_cur(sv) STMT_START { \

perl5/toke.c

Line 11876 in 7946d6a

SvLEN_set(sv, SvCUR(sv) + 1);

0e72d90

khwilliamson committed on Aug 22, 2020
toke.c: Use SvPV_shrink_to_cur

b7e9a5c

nwc10 committed on Apr 18, 2005
Replace Renew(SvPVX(...)...) with SvPV_renew, which avoids an LVALUE

93a17b2#diff-eab4115a843bf91f2d20457b59e6b0c6c747ffa64ba1022d3ff958512da8df12R3612

Larry Wall committed on Oct 9, 1993
perl 5.0 alpha 3

3001ef3#diff-dc8c62daf844c4b19510dfce777053a52ac4821852ef9818e3de8912dd3c4edbR113-L8440

khwilliamson committed on Feb 27, 2016
regcomp.c: Improve free-ing up unused inversion list space

slightly related to #22623

What is going on inside S_invlist_trim()??? 1 out of 4 BP hits while running mktables shows SvPV_renew() is getting called on new len value 9, while SvLEN() says it already is 9 long. A trip through libc realloc() for no reason at all. Windows OS'es malloc() impl always will move the 9 byte mem block and return a new ptr every time in my observation, and alternate between the exact same 2 ptrs addrs if someone does a NOOP like that. Other 50% or 75% of BP hits S_invlist_trim() were a number like 0x38 or 0x3C. but its approx 0x3_. Forgot the last digit. So 0x38 -> 0x09 sounds correct to me. Its atleast 2*PTRSIZE delta. so it will jump buckets on most OSes probably.

Why is it 0x09 and not 8/sizeof(UV) anyway? SVt_INVLIST objs are undocumented for CPAN and unreachable from PP. And if the first 4/8 bytes are NULL, its already nul term-ed and unprintable binary. 0x09 vs 0x8 matters for WinPerl since its P5P created mem pool API has a 3 x void*/24 byte long header, and reverse engineering/hex dump proves on my particular Win64 build number, I have an 8 byte OS malloc header in AMD64 mode. Not sure if that means the minimum end user malloc() bucket size on Win64 is just 8 bytes and not 16 bytes or 24 bytes, and increases in 8 byte units, not 2 x PTRSIZE (16) byte units. These assumptions (bucket sizes/granularity/alignment aka SSE drama, 8 bytes vs 16 bytes malloc() internals on 64b CPU) are generally way too OS vendor+build number+security update specific, to hard code into the Perl interp. Easier to get rid of the +1 nul char instead of calculating if it wastes 0 bytes, 7 or 15 bytes on a certain line number on whose server or PC or phone.

Anyways, the +1 on a non-text C struct, and then wasting 15 more bytes, is a persistent bug hazard in the Perl C API. See #22879

This is a smoke test, or bug report, not a PR.

---

• This set of changes does not require a perldelta entry.

[richardleach]

At first glance, looks like overlap with #23103 ?

#23111 was intended as a very uncontroversial initial step for that issue, straightforward enough that it could be applied early while the longer term approach was discussed.

[bulk88]

At first glance, looks like overlap with #23103 ?

#23111 was intended as a very uncontroversial initial step for that issue, straightforward enough that it could be applied early while the longer term approach was discussed.

Yeah. You identified the bug before me, but your PRs have been stalled for a long time, long enough I found the bug again with a profiler report by accident. My OP in this ticket describes what to do with S_invlist_trim().

stage 1 fix is add bounds checking back to S_invlist_trim() which was lost in 2020 (? chk OP). stage 1 is ASAP since its seen in production code on stable perl and unacceptable, maint release required IMO. Loss of bounds check mistake was made in 2020 (?) or 2016.

stage 2 is the add the assert in this PR/demo, since SvPV_renew() and SVPV_shinktocur() are starting to get mis/abused, and P5P C ppl don't know how to use them safely or correctly. Sooner or later, someone will push to blead code calling those 2 funcs on any 1 of 4 SV COW types, on SVPV OOK, on HvARRAY or AvARRAY.

Stage 3 is difficult strategic long term action with parts of the interp that are believe they are, plan to or are, executing with O(n^2) or O(n^n) and malloc() that much memory on entry, and then SvPV_renew() it away.

SvPV_renew() and SVPV_shinktocur() are becoming interp core's very own XKCD/SO comic book of offshore IT contractors, similar to using
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/msize?view=msvc-170 and https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-isbadreadptr and https://learn.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapvalidate to fix memory corruption (and still failing to know what your doing) or https://linux.die.net/man/3/mprobe

stage 4 is dealing with that WinOS's official realloc() doesn't shrink since its UB per ISO C. Only free() releases memory per ISO C. If Perl really needs SvPV_renew()/SVPV_shinktocur() and its not code rot, a formal malloc() memcpy() free() impl needs to exist for WinPerl. And alot of testing / experiments / benchmars are needed here.

MS (or a citizen) finally seems to have public API documented shrinking a malloc() block in place, can't won't and will never happen on windows

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/expand?view=msvc-170

Its fundamentally impossible to shrink in place with a true bucket size allocator, that leaves 2 or 3 bits free, in the header/metadata, to record "unused" bytes for msize() fakery and free() tail write overflow sentinal checking. If the malloc ptr is "in the range"/bounds check of that internal bucket array, its guaranteed what size by ptr address what the original size was of the malloc() block, because 2 adjacent blocks can't be combined under a strict bucket allocator. Realistic implementations probably allow in place binding 1 more adjacent bucket to the previous block to grow in place, but will bucket jump if >2 current bucket size, or next block is busy.

WinOS's actual malloc() allocator algorithm isn't super important to WinPerl, since HeapAlloc()/malloc() private internals get changed every 2-3 years by Microsoft since Win2K was released, and before that bizarrely MS CRT refused to use kernel32's HeapAlloc() and DIYed their own, <= MSVC 6. IDK y. MS CRT's private DIYed malloc()'s code was deleted and gone in Win2K's OS .iso "private undocumented" msvcrt.dll/MSVC 2002 (VC6+1).

Perl only needs to make a few basic assumptions that are back/future proof that work on all OSes. Not 1 build number of FreeBSD or Glibc. PTRSIZE or just 8 b/c of NV/double is a safe assumption. the malloc header, if it actually is right behind the end user ptr, will be PTRSIZE at minimum. Modern high security for-profit OSes are starting to keep malloc header metadata in a separate array at a random virtual address but a fixed offset from the end user ptr. 1000s of CVEs of bad writes/buffer overflows rewriting the next malloc header causing a blackhat exploit have made that rule written in IT blood. Its questionable if the metadata is 2*PTRSIZE or PTRSIZE, But irrelevant for Perl. Not keeping a +1 for '\0' on a C struct or non text array, wasting 7 bytes, is pretty basic coding skills. Rounding up to the next 8 or 16 for a PV buffer if it is a string, to stop round trips through realloc() is pretty basic coding skills. sv.c uses 12/16 on all OSes and CPUs out of the box.

/* Functions like Perl_sv_grow mandate a minimum string size.
 * This was 10 bytes for a long time, the rationale for which seems lost
 * to the mists of time. However, this does not correlate to what modern
 * malloc implementations will actually return, in particular the fact
 * that chunks are almost certainly some multiple of pointer size. The
 * default has therefore been revised to a more useful approximation.
 * Notes: The following is specifically conservative for 64 bit, since
 * most dlmalloc derivatives seem to serve a 3xPTRSIZE minimum chunk,
 * so the below perhaps should be:
 *     ((PTRSIZE == 4) ? 12 : 24)
 * Configure probes for malloc_good_size, malloc_actual_size etc.
 * could be revised to record the actual minimum chunk size, to which
 * PERL_STRLEN_NEW_MIN could then be set.
 */
#ifndef PERL_STRLEN_NEW_MIN
#define PERL_STRLEN_NEW_MIN ((PTRSIZE == 4) ? 12 : 16)
#endif

/* Round all values passed to malloc up, by default to a multiple of
   sizeof(size_t)
*/
#ifndef PERL_STRLEN_ROUNDUP_QUANTUM
#define PERL_STRLEN_ROUNDUP_QUANTUM Size_t_size