On 12/19/2014 04:30 PM, bulk88 wrote:
> Reini Urban wrote:
>> On 12/19/2014 02:25 PM, A. Sinan Unur wrote:
>>  
>>> On Fri, Dec 19, 2014 at 3:00 PM, A. Sinan Unur <na...@cpan.org> wrote:
>>>    
>>>> On Fri, Dec 19, 2014 at 2:04 PM, A. Sinan Unur <na...@cpan.org> wrote:
>>>>      
>>>>> On Fri, Dec 19, 2014 at 11:08 AM, Nigel Horne <n...@bandsman.co.uk>
>>>>> wrote:
>>>>>        
>>>>>> PS – it would help if the red column under the MSWin32 column of
>>>>>> http://matrix.cpantesters.org/?dist=POE-Component-SmokeBox+0.48
>>>>>> could be
>>>>>> addressed.
>>>>>>           
>>>> OK, some more progress ... Here is the stack when t\backend\02_check.t
>>>> fails. We have a null pointer thing going on:
>>>>
>>>> First-chance exception at 0x0000000054BD04C7 (perl520.dll) in
>>>> perl.exe: 0xC0000005: Access violation writing location
>>>> 0x0000000000000000.
>>>> Unhandled exception at 0x0000000054BD04C7 (perl520.dll) in perl.exe:
>>>> 0xC0000005: Access violation writing location 0x0000000000000000.
>>>>       
>>> Hmmmmm .... Could it be related to:
>>>
>>> http://perl5.git.perl.org/perl.git/commitdiff/1cbe4e6fdc25527a4e9930e2f6a96fea3834bfd1?hp=8fe523b46877224b0d8f3bd5ea78396ff0b4242d
>>>
>>>
>>> SvREFCNT_dec_NN in SDBM
>>>
>>> VC 2003 optimizer didn't catch it because SvREFCNT_dec is rarely inlined
>>> on -O1
>>>     
>>
>> Yes, bulk88's _NN patch is wrong. You need to catch the NULL here
>>   
> 
> My patch was correct. "if(pointer){if(!pointer) {unreachable();}}" If
> suddenly unreachable() can be called, you need to submit a bug report to
> your CC vendor.

Ok, I see.
But SDBM can still use an easier approach to fix this global destruction
problem in the fork.

Just check the pointers for being NULL already. This is permitted on
linux, but not on Windows. Which is where I'm coming back to your _NN
patch. You need to catch the NULL here and cannot optimize it away.

> The callstack is from 5.20.*, that p5 core patch is in 5.21.7. Its not
> on Unur's machine. Since the callstack had symbols, it is easy to figure
> out what went wrong.
> 
> perl520.dll!VMem::Free(void * pMem) Line 202    C++
> perl520.dll!Perl_safesysfree(void * where) Line 360    C
> SDBM_File.dll!XS_SDBM_File_DESTROY(interpreter * my_perl, cv * cv) Line
> 262    C
> perl520.dll!Perl_pp_entersub(interpreter * my_perl) Line 2797    C
> ..........
> perl520.dll!perl_destruct(interpreter * my_perl) Line 807    C
> perl520.dll!win32_start_child(void * arg) Line 1796    C++
> 
> Line 202, http://perl5.git.perl.org/perl.git/blob/HEAD:/win32/vmem.h#l202
> 197         if (ptr->owner != this) {
> 198             if (ptr->owner) {
> 199 #if 1
> 200                 int *nowhere = NULL;
> 201                 Perl_warn_nocontext("Free to wrong pool %p not
> %p",this,ptr->owner);
> 202                 *nowhere = 0; /* this segfault is deliberate,
> 203                                  so you can see the stack trace */
> 204 #else
> 205                 ptr->owner->Free(pMem); 206 #endif
> 207             }
> 
> SDBM_File is ithread and therefore psuedofork unsafe. It either needs a
> CLONE method in PP, svt_dup magic, and/or MY_CXT or to fatally die if a
> resource can't be duplicated and a CLONE is attempted.
> 
> void
> sdbm_DESTROY(db)
>     SDBM_File    db
>     CODE:
>     if (db) {
>         int i = store_value;
>         sdbm_close(db->dbp);
>         do {
>         if (db->filter[i])
>             SvREFCNT_dec(db->filter[i]);
>         } while (i-- > 0);
>         safefree(db) ;
>     }
> ------------------------------------------
> SDBM_File        T_PTROBJ
> ------------------------------------------
> T_PTROBJ
>     if (SvROK($arg) && sv_derived_from($arg, \"${ntype}\")) {
>         IV tmp = SvIV((SV*)SvRV($arg));
>         $var = INT2PTR($type,tmp);
>     }
>     else
>         Perl_croak(aTHX_ \"%s: %s is not of type %s\",
>             ${$ALIAS?\q[GvNAME(CvGV(cv))]:\qq[\"$pname\"]},
>             \"$var\", \"$ntype\")
> ------------------------------------------
> 
> SDBM_File
> sdbm_TIEHASH(dbtype, filename, flags, mode, pagname=NULL)
>     char *        dbtype
>     char *        filename
>     int        flags
>     int        mode
>     char *        pagname
>     CODE:
>     {
>         DBM *     dbp ;
> 
>         RETVAL = NULL ;
>         if (pagname == NULL) {
>             dbp = sdbm_open(filename, flags, mode);
>         }
>         else {
>             dbp = sdbm_prep(filename, pagname, flags, mode);
>         }
>         if (dbp) {
>             RETVAL = (SDBM_File)safecalloc(1, sizeof(SDBM_File_type));
>         RETVAL->dbp = dbp ;
>         }
>             }
>     OUTPUT:
>       RETVAL
> -------------------------------------------
> Yep, thread unsafe. If instead of using a blessed SVIV, it used a
> blessed SVPV and sv_grow/SvCUR_set, perl would automatically dup the
> memory block, but it would still be unsafe since the memory block is
> 
> typedef struct {
>     DBM *     dbp ;
>     SV *    filter[4];
>     int     filtering ;
>     } SDBM_File_type;
> 
> and it keeps SV*s there that Perl doesn't know about at CLONE time.
> 
> Now even if SDBM_File_type was replaced with an AV, and AvARRAY and
> SvIVX for speed, you will still crash when
> 
> ------------------------------------------
>         sdbm_close(db->dbp);
> ------------------------------------------
> 
> runs twice on the same "DBM *     dbp". I dont see how this can ever be
> thread safe. IMO setting the blessed SVIV object to 0 during a CLONE is
> the only thing to do, unless you can duplicate the data/resources inside
> DBM * through the DBMS's API. SDBM would fail with valgrind and ithreads
> on Linux with a double free anyway inside sdbm_close().



Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to