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

Panu Matilainen pmatilai at redhat.com
Wed Oct 4 07:41:12 UTC 2017


On 10/03/2017 08:12 PM, Thierry Vignaud wrote:
> On 3 October 2017 at 07:34, 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. 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 slightly disagree. IMHO that's a rpm-4.14 regression in the case of urpmi:
> - header was used to be initialized by calling rpmReadPackageFile()
> - this has worked for a decade
> - this works smoothly if I replace the bogus pkgs by good ones
> - header is no more initialized only for bogus packages, which is hit
> by the testsuite
> 

I was going to say it's relying on undocumented behavior, but something 
told me to check my facts before posting :) Turns out you're the one 
who's correct here:

/** \ingroup header
  * Return package header from file handle, verifying digests/signatures.
  * @param ts            transaction set
  * @param fd            file handle
  * @param fn            file name
  * @retval hdrp         address of header (or NULL)
  * @return              RPMRC_OK on success
  */
rpmRC rpmReadPackageFile(rpmts ts, FD_t fd, const char * fn,
                          Header * hdrp);

So it is actually documented behavior, and a ~15min empirical study 
shows that roughly half of the 3rd party API users from my sample 
actually rely on it.

> Anyway I workarounded it with:
> http://gitweb.mageia.org/software/rpm/perl-URPM/commit/?id=87dbde4f3b078173e53cd45cac000c2d2751b370
> But I still smell a bad regression in the way librpm treats bogus rpms.
> 

It being documented makes it a no-brainer, but just the fact that so 
much third party software relies on it would've been enough to warrant 
reinstating the old behavior, especially since it's on the 
security-sensitive path, and no way to issue compiler warnings on the 
change either AFAICS.

Fixed now in both git master and 4.14.x branch, thanks for reporting and 
persevering :)

	- Panu -


More information about the Rpm-maint mailing list