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

[Thierry Vignaud]

On 4 October 2017 at 09:41, 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 :)

Thx for fixing it :-)