[Rpm-maint] rpm4.14 makes perl-RPM4 testsuite to segfault
Panu Matilainen
pmatilai at redhat.com
Thu Oct 5 07:28:01 UTC 2017
On 10/05/2017 09:34 AM, Panu Matilainen wrote:
> On 10/04/2017 06:59 PM, Thierry Vignaud wrote:
>> On 4 October 2017 at 14:17, Thierry Vignaud
>> <thierry.vignaud at gmail.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
>>
>> You can test by building the perl-RPM4 packages with the attached spec
>> file, w/o installing it.
>> Sources are at
>> http://search.cpan.org/CPAN/authors/id/T/TV/TVIGNAUD/RPM4-0.36.tar.gz
>> then in order to run the broken test with the just compiled module:
>> cd BUILD/RPM4-0.36/
>> PERL_DL_NONLAZY=1 PERL_USE_UNSAFE_INC=1 gdb -q --args perl
>> "-MExtUtils::Command::MM" "-MTest::Harness" -Iblib/lib -Iblib/arch
>> t/05transaction.t
>
> Oh, that's helpful, thanks. I managed to get perl-RPM4 setup & built in
> a way that allows me to bisect in a fairly reasonable manner but didn't
> figure out how to run tests without involving make (which makes actually
> debugging stuff quite a pita).
>
>> What's interesting is that:
>> - it segfaults on FC26 with rpm-4.13.0.1-7.fc26.x86_64
>> - it doesn't segfaults but show failures on mga6 with
>> rpm-4.13.0.1-3.1.mga6
> >
>> Some patch difference could explain that
>> I see that it hasn't been rebuild for rpm updates between
>> 4.13.0-0.rc1.32.mga6 & 4.13.0.1-3.mga6 and I guess the other
>> regressions weren't checked for after a signing segfault in the
>> testsuite due to rpm-4.13 was fixed
>>
>
> Yeah, I'm getting segfaults all the way to rpm 4.11.x, didn't test
> earlier because this already shows it's not a regression in 4.14.x but
> something else. A bug in perl-RPM4 perhaps, as compiling it with -Og
> makes the crash go away, other optimization levels make it blow up with
> different levels of spectacular. I dont see anything obvious in there
> but that doesn't mean much, I know diddly about perl and its extensions.
I ran it with some added debugging on rpm side which I'm more familiar
with, and the crash happens because a totally garbage pointer is passed
to headerFree(). Well indeed, it was passing the address of the header
pointer variable as the header itself into the callback, and when you
try do stuff with that, well...
This fixes it:
diff --git a/src/RPM4.xs b/src/RPM4.xs
index 04c65ee..6604477 100644
--- a/src/RPM4.xs
+++ b/src/RPM4.xs
@@ -246,7 +246,7 @@ static void *
s_what = "INST_START";
if (h) {
mXPUSHs(newSVpv("header", 0));
- mXPUSHs(sv_setref_pv(newSVpvs(""), bless_header, &h));
+ mXPUSHs(sv_setref_pv(newSVpvs(""), bless_header, h));
#ifdef HDRPMMEM
PRINTF_NEW(bless_header, &h, -1);
#endif
- Panu -
More information about the Rpm-maint
mailing list