Re: Patch to fix bad version comparison in RPMPackageCheck()

[Phil D'Amore]

Hmmm.  Interesting.  The check on the return value should be cleaned up
to better conform to what strcmp advertises (I had previously just
trusted rpmvercmp's advertised return values).

I have to say, though, that I can't produce a case where strcmp returns
anything other than -1, 0, or -1, despite the more vague return value
description in the manpage.

Have you found a case where strcmp() returns something other than that
on one of your machines?

On Mon, 2005-02-28 at 13:46, address@hidden wrote:
> Hi Phil,
> 
> Even with your latest patch, there are bugs when comparing version
> and release strings...  when checking versions and releases,
> RPMPackageCheck() assumes rpmvercmp() only returns -1,0,1... eg...
> 
>    switch (rpmvercmp(vA, vB))
>      {
>      case 1:    result = cmpsense_gt;
>                 break;
>      case -1:   result = cmpsense_lt;
>                 break;
>      }
> 
> ...but rpmvercmp() can return *other results* by way of strcmp()...
> 
>     /* strcmp will return which one is greater - even if the two */
>     /* segments are alpha or if they are numeric.  don't return  */
>     /* if they are equal because there might be more segments to */
>     /* compare */
>     
>     rc = strcmp(one, two);
>     if (rc)
>        {
>        free(s1);
>        free(s2);
>        return rc;
>        }
> 
> Case in point: back on Oct 23rd of 2004 I posted a cfagent script
> that shows a bug with glibc-2.2.5-42 vs glibc-2.2.5-44 on RedHat 7.3,
> and note that, on RedHat 7.3, strcmp("42","44") returns -2.
> 
> steve 
> - - - 
> systems & network manager
> high energy physics
> university of wisconsin
> 
>  > ---- Original Message ----
>  > From: "Phil D'Amore"
>  > 
>  > I was reviewing the code in RPMPackageCheck() to see if there was anything 
>  > fundamentally wrong with the way it did version comparison.  What I 
>  > found is fixed in the attached patch.
>  > 
>  > Basically, when comparing versions, it compares each of the three 
>  > components, epoch, version, and release, in order.  Since this goes from 
>  > most significant to least, as soon as it finds one component that 
>  > differs, it should be using that result as the result of the 
>  > comparison.  In reality, the code would just plow through, doing the 
>  > other comparisons, and the result of the last comparison done would be 
>  > returned.  For example:
>  > 
>  > Requested - 4.12-38
>  > Installed - 4.13-37
>  > 
>  > The installed version is greater than what is requested, because 4.12 < 
>  > 4.13.  However, after doing that comparison, instead of ignoring the 
>  > difference in the release portion (38 vs 37), it does the comparison, 
>  > and since 38 > 37, it would incorrectly decide that the installed 
>  > package version is *less* than what is requested, which can obviously 
>  > cause problems if you are being very specific about the version you are 
>  > looking for.
>  > 
>  > The attached patch fixes things so the comparison correctly stops after 
>  > seeing 4.12 < 4.13, since at that point the -38 and -37 components are 
>  > irrelevant.
>  > 
>  > I suspect most folks don't get this specific, so it has not really been 
>  > a problem.  I know I personally have never run into this problem on my 
>  > production systems.  Still, it should be fixed, so here ya go...
>  > 
>  > Thanks,
>  > 
>  > -- 
>  > Phil D'Amore                             "Sometimes there is a fine line
>  > Senior System Administrator               between criminally abusive
>  > Red Hat, Inc                              behavior and fun."
>  > Office: 919.754.3700 x44395                 -- Ted the Generic Guy
>  > Pager: 877.383.8795                            (Dilbert 4/19/2003)
>  > 
-- 
Phil D'Amore                             "Sometimes there is a fine line
Senior System Administrator               between criminally abusive
Red Hat, Inc                              behavior and fun."
Office: 919.754.3700 x44395                 -- Ted the Generic Guy
Pager: 877.383.8795                            (Dilbert 4/19/2003)