[Rpm-maint] rpm4.14 makes perl-RPM4 testsuite to segfault (was: Re: [Rpm-announce] RPM 4.14.0 release candidate 2 is out)

Wed Oct 4 12:17:05 UTC 2017

On 4 October 2017 at 13:26, Panu Matilainen <pmatilai at redhat.com> wrote:
>>>>>>>> Also this new rpm introduced segfault regressions in both RPM4 &
>>>>>>>> urpmi
>>>>>>>> testsuites
>>>>>>>> See attached gdb traces in BUG*.txt
>>>>>>>> valgrind seems to hint about invalid writes/reads
>>>>>>>> See you
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The urpmi issue is when checking bogus pkgs.
>>>>>>> The RPM4 issue is when traversing the transaction (not the rpmdb)
>>>>>>> Attached are the valgrind outputs
>>>>>>>
>>>>>>
>>>>>> So we have stuff like
>>>>>>
>>>>>> ==14087== Invalid write of size 4
>>>>>> ==14087==    at 0x103AA6DD: headerUnlink (header.c:188)
>>>>>> ==14087==    by 0x103AA6DD: headerFree (header.c:194)
>>>>>> ==14087==    by 0xFF69314: XS_RPM4__Header_DESTROY (RPM4.xs:890)
>>>>>> ==14087==    by 0x3F512E2C40: Perl_pp_entersub (pp_hot.c:4231)
>>>>>> ==14087==    by 0x3F5125551E: Perl_call_sv (perl.c:2848)
>>>>>> ==14087==    by 0x3F512E7C09: S_curse (sv.c:6987)
>>>>>> ==14087==    by 0x3F512E84F7: Perl_sv_clear (sv.c:6591)
>>>>>> ==14087==    by 0x3F512E898D: Perl_sv_free2 (sv.c:7088)
>>>>>> ==14087==    by 0x3F513182E6: UnknownInlinedFun (inline.h:200)
>>>>>> ==14087==    by 0x3F513182E6: Perl_free_tmps (scope.c:212)
>>>>>> ==14087==    by 0x3F512DAD74: Perl_pp_nextstate (pp_hot.c:52)
>>>>>> ==14087==    by 0x3F512DAA55: Perl_runops_standard (run.c:41)
>>>>>> ==14087==    by 0x3F5125D236: S_run_body (perl.c:2524)
>>>>>> ==14087==    by 0x3F5125D236: perl_run (perl.c:2447)
>>>>>> ==14087==    by 0x400C79: main (perlmain.c:123)
>>>>>> ==14087==  Address 0xffeffef8c is on thread 1's stack
>>>>>> ==14087==  396 bytes below stack pointer
>>>>>>
>>>>>> ...and all the failures are around headerFree(), but none of the
>>>>>> traces
>>>>>> go
>>>>>> into rpm itself, so I dont really know what does "traversing the
>>>>>> transaction" actually mean.
>>>>>
>>>>>
>>>>>
>>>>> in both URPM & RPM4 bindings, there's a family of traverse functions
>>>>> that enable to execute a callback on each package of either:
>>>>> - rpmdb
>>>>> - the current transaction,
>>>>> - pkgs header from an hdlist file
>>>>> - synthetic metadata from a synthesis file
>>>>> calling the callback on the currrent header
>>>>>
>>>>> See:
>>>>> - Db_traverse*() & Trans_traverse() in perl-URPM/URPM.xs
>>>>> - Ts_traverse() in RPM4-0.36/src/RPM4.xs
>>>>>
>>>>> It's loosely documented at
>>>>> http://search.cpan.org/~tvignaud/URPM-5.12/URPM.pm or
>>>>>
>>>>>
>>>>> http://search.cpan.org/~tvignaud/RPM4-0.36/lib/RPM4/Transaction.pm#Hdlist::Db-%3Etraverse_headers(sub)
>>>>> (RPM4's doc is very incomplete -- I'm only maintaining this b/c other
>>>>> tools depend on that and I cannot break them when upgrading rpm)
>>>>>
>>>>>> But the problem is simply with perl-RPM4 and
>>>>>> urpmi passing uninitialized variables to headerFree().
>>>>>>
>>>>>> What changed in rpm is that rpmReadPackageFile() no longer does this
>>>>>> as
>>>>>> the
>>>>>> first thing:
>>>>>>
>>>>>>       if (hdrp)
>>>>>>           *hdrp = NULL;
>>>>>>
>>>>>> Ie if you pass an uninitialized pointer as hdrp, it remains
>>>>>> uninitialized
>>>>>> unless rpmReadPackageFile() returns with a success code.
>>>>>> Which is how I think it should be, but it does deserve a release note
>>>>>> on
>>>>>> the
>>>>>> changed API.
>>>>>>
>>>>>> So the moral of the story is basically: if you depend on your
>>>>>> variables
>>>>>> being initialized, initialize them by yourself. It's a good practise
>>>>>> anyway.
>>>>>
>>>>>
>>>>>
>>>>> I'll check for uninitialized variables later today
>>>>
>>>>
>>>>
>>>> Actually the bug happen when running the transaction
>>>>
>>>
>>> Right, that makes sense, there's a second rpmReadPackageFile() inside
>>> transaction run.
>>
>>
>> Intestingly, perl-RPM4 no more segfaults (there still faillures
>> though) if I alter the transrun() method (which calls rpmtsRun in
>> rpmlib) 's perl callback to *not* call std rpmShowProgress())
>> See attached patch
>>
>> Aka previously it runs both the passed perl callback + rpmlib default
>> callback which worked smoothly.
>> But since rpm-4.14, calling std rpm callback results a segfault...
>> I'm happy to not call that and I don't think we have any tool actually
>> running transactions^W installing packages through RPM4 right now but
>> still that's a regression in rpm...
>> Maybe some new unlink call or sg like that
>>
>
> Are you saying you still get that segfault with this fix?
> https://github.com/rpm-software-management/rpm/commit/082e6e77dd613efa643b02ee0417c1382f520893

That fix was for URPM segfaulting while verifying bad rpms, here I'm
talking about RPM4 segfaulting while installing packages
(different binding, different issue)
The URPM issue is fixed by the above commit (s/is/should/ - I hadn't
actually tested your fix)
The RPM4 issue is a different issue, even if both happen in headerFree() .
You might get confused by the perl gdb traces that looks similar but
the issues really are different.
They show the point where the perl interpreter segfault, but not the
point where the call in rpmlib has garbaged things as this action has
finished and we'd return to the perl interpreter.

> From the traceback you posted it was tripping on the uninitialized header
> pointer as the other one. And if you disable the entire callback then there
> are no open files returned and the whole error path is different.

There's still a perl callback that's run.
Actually there's a C/XS wrapper (transCallback()), running the
optional pure perl callback if present
Ts_transrun => rpmtsSetNotifyCallback with transCallback wrapper as
callback and the actual perl callback as parameter
(similar to what URPM is doing)
Then during the actual installation, rpmlib calls the callback
(transCallback) which set up the perl arg stack & calls the perl
callback which itself used to call the rpmlib default callback.

Here, the segfault is also in headerFree() but only happen in the
install callback while installing pkgs and only if the perl callback
calls the default rpmlib progress callback.

> BTW, your workaround in
> http://gitweb.mageia.org/software/rpm/perl-URPM/commit/?id=87dbde4f3b078173e53cd45cac000c2d2751b370
> is not the one you want: initialize to NULL instead

Ack. Thx
Though from what you said, I should be able to remove any
initialization at all as the rpmReadPackageFile() regression is fixed
and should now always set the return parameter.
See you