Safefree() in a win32 “subprocess” blows up the process

[FGasper]

Description
Safefree() in a win32 “subprocess” misbehaves when memory was first allocated in “parent”.

Steps to Reproduce
On Win32: Clone https://github.com/FGasper/p5-win32-safefree-bug, then perl Makefile.PL && gmake && perl -Mblib t\detect_memory_leaks.t.

Expected behavior
The test should finish, as it does in Linux. Instead it fails with “Free to wrong pool …”.

Perl configuration
Strawberry 5.30.1 64-bit …

Summary of my perl5 (revision 5 version 30 subversion 1) configuration:

  Platform:
    osname=MSWin32
    osvers=10.0.18363.476
    archname=MSWin32-x64-multi-thread
    uname='Win32 strawberry-perl 5.30.1.1 #1 Fri Nov 22 02:24:29 2019 x64'
    config_args='undef'
    hint=recommended
    useposix=true
    d_sigaction=undef
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=undef
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
    bincompat5005=undef
  Compiler:
    cc='gcc'
    ccflags =' -s -O2 -DWIN32 -DWIN64 -DCONSERVATIVE -D__USE_MINGW_ANSI_STDIO -DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO -fwrapv -fno-strict-aliasing -mms-bitfields'
    optimize='-s -O2'
    cppflags='-DWIN32'
    ccversion=''
    gccversion='8.3.0'
    gccosandvers=''
    intsize=4
    longsize=4
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='long long'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='g++.exe'
    ldflags ='-s -L"C:\Users\cpanel\Desktop\strawberry-perl-5.30.1.1-64bit-portable\perl\lib\CORE" -L"C:\Users\cpanel\Desktop\strawberry-perl-5.30.1.1-64bit-portable\c\lib"'
    libpth=C:\Users\cpanel\Desktop\strawberry-perl-5.30.1.1-64bit-portable\c\lib C:\Users\cpanel\Desktop\strawberry-perl-5.30.1.1-64bit-portable\c\x86_64-w64-mingw32\lib C:\Users\cpanel\Desktop\strawberry-perl-5.30.1.1-64bit-portable\c\lib\gcc\x86_64-w64-mingw32\8.3.0
    libs= -lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    perllibs= -lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    libc=
    so=dll
    useshrplib=true
    libperl=libperl530.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs
    dlext=xs.dll
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags='-mdll -s -L"C:\Users\cpanel\Desktop\strawberry-perl-5.30.1.1-64bit-portable\perl\lib\CORE" -L"C:\Users\cpanel\Desktop\strawberry-perl-5.30.1.1-64bit-portable\c\lib"'


Characteristics of this binary (from libperl):
  Compile-time options:
    HAS_TIMES
    HAVE_INTERP_INTERN
    MULTIPLICITY
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_IMPLICIT_CONTEXT
    PERL_IMPLICIT_SYS
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    USE_64_BIT_INT
    USE_ITHREADS
    USE_LARGE_FILES
    USE_LOCALE
    USE_LOCALE_COLLATE
    USE_LOCALE_CTYPE
    USE_LOCALE_NUMERIC
    USE_LOCALE_TIME
    USE_PERLIO
    USE_PERL_ATOF
  Built under MSWin32
  Compiled at Nov 22 2019 02:30:43
  @INC:
    C:/Users/cpanel/Desktop/strawberry-perl-5.30.1.1-64bit-portable/perl/site/lib
    C:/Users/cpanel/Desktop/strawberry-perl-5.30.1.1-64bit-portable/perl/vendor/lib
    C:/Users/cpanel/Desktop/strawberry-perl-5.30.1.1-64bit-portable/perl/lib

[xenu]

It's not a bug, it was implemented that way on purpose. The whole point of the "Free to wrong pool" error is to tell you that a wrong thread is freeing the memory.

Whether it makes sense or not is a different question. Personally, I'm not familiar enough with that code to have an opinion.

[FGasper]

@xenu So are all XSUBs then on the hook to detect when they’re in a pseudo-subprocess and forgo Safefree(), then?

I thought the notion of Safefree() was to abstract away details like that …

[Leont]

Whether it makes sense or not is a different question. Personally, I'm not familiar enough with that code to have an opinion.

I don't think that the people who wrote this still around. I have also been bitten by this, and resorted to using PerlMemShared_malloc and friends, but it's quite unhelpful.

I had assumed it's because perl is using LocalAlloc, but that doesn't appear to be the case. Quite frankly it rather looks to me like we could remove this logic and nothing stops working.

My second guess is that it prevents memory leaks from pseudo-fork processes to affect the other "processes", but in that case it's really not worth the hassle IMO.

You could try to compile perl with this patch, and see if anything breaks (I suspect not)

Author: Leon Timmermans <fawaka@gmail.com>
Date:   Sun Jan 5 16:28:38 2020 +0100

    Don't use thread specific memory pools

diff --git win32/vmem.h win32/vmem.h
index 3fd7e169fc..372661944f 100644
--- win32/vmem.h
+++ win32/vmem.h
@@ -22,7 +22,7 @@
 #define ___VMEM_H_INC___
 
 #define _USE_MSVCRT_MEM_ALLOC
-#define _USE_LINKED_LIST
+// #define _USE_LINKED_LIST
 
 // #define _USE_BUDDY_BLOCKS

[xenu]

BTW, note that this behavior is not specific to Windows. Unix perls built with -DDEBUGGING do the same thing (of course, that only affects threads because non-win32 perls don't have pseudoprocesses).

[FGasper]

@xenu OK, thank you; that was my next question: whether this avoid-Safefree-if-in-subprocess behavior is specific to win32, or if there’s a broader category of “pseudoprocess-using perls”.

@Leont Off-hand I don’t know how to compile a win32 perl. I’ll look, though. Thank you!

[xenu]

if there’s a broader category of “pseudoprocess-using perls”.

I believe AmigaOS port has pseudoprocesses too, but their implementation is completely separate from the Windows one and I have no idea how it works.

[tonycoz]

I think this needs documentation rather than a code change. I had thought this was documented, but I don't see anything.

In general if you want memory that isn't tied to the current thread you should be using safesysmalloc()/safesysrealloc()/safesysfree().

[Leont]

I think this needs documentation rather than a code change.

But is there any reason for this behavior? Because quite frankly I've only seen it get in the way of things.

[FGasper]

In general if you want memory that isn't tied to the current thread you should be using safesysmalloc()/safesysrealloc()/safesysfree().

@tonycoz These functions are undocumented … should they be added to perlapi/perlclib?

[tonycoz]

it has bitten me before too (which is why most of Imager avoids the perl headers, since malloc() is redefined to PerlMem_malloc() on Win32).

I guess it depends on why it was added, if we do look at removing it I think it needs to be discussed on the list rather than just in this ticket, so whoever added it has an opportunity to defend it.

[tonycoz]

@tonycoz These functions are undocumented … should they be added to perlapi/perlclib?

Yes.

[iabyn]

On Sun, Jan 05, 2020 at 06:58:56PM -0800, Tony Cook wrote: I guess it depends on why it was added, if we do look at removing it I think it needs to be discussed on the list rather than just in this ticket, so whoever added it has an opportunity to defend it.

My recollection is that (at the time, at least: 2005), Windows would give a "free to wrong pool" error if memory malloc()ed by one thread was free()d by another thread. Jan DuBois then added a mechanism (by default only enabled on DEBUGGING+PERL_IMPLICIT_CONTEXT builds) that on *all* platforms recorded thread info with each allocation, and croaked if freed in a different thread - thus helping non-Windows coders like myself from introducing bugs that tested fine on Linux. The thread where it was introduced is here: http://nntp.perl.org/group/perl.perl5.porters/106649 malloc() on Linux appears thread-safe and includes mechanisms to reduce lock contention. I don't know about other OSes.

…

-- Justice is when you get what you deserve. Law is when you get what you pay for.

[Leont]

My recollection is that (at the time, at least: 2005), Windows would give a
"free to wrong pool" error if memory malloc()ed by one thread was free()d
by another thread.

TBH that sounds to me like a case of " No, it is not a compiler error. It is never a compiler error." I really don't believe that malloc can be so unhelpful without this widely being known and documented. Google tells me msvc malloc isn't thread safe unless one links with the thread-safe c library, which might be the problem observed back then.

[tonycoz]

My recollection is that (at the time, at least: 2005), Windows would give a "free to wrong pool" error if memory malloc()ed by one thread was free()d by another thread. ...

This is produced by perl, not the underlying malloc() implementation, see https://github.com/Perl/perl5/blob/blead/win32/vmem.h#L199

Both shared and "system" memory end up being allocated/freed with malloc()/free(), with shared memory having a VMem object and each thread having their own VMem objects.

I suspect the intent was to allow fork emulation to clean up all of the (perl) allocated memory from a child thread, but I'm only guessing here.

This comes with a fair amount of overhead on Win32, even on non-debug builds, each memory block has a next and prev pointers for the double-linked list (8-16 bytes), and extra locking (which the underlying malloc() also presumably already does.)

I'm inclined to remove this, but I'd rather not do it blindly.