[PATCH] cygcheck: Fix parsing of file names containing colons

[Ken Brown]

On 10/25/2017 10:19 AM, Corinna Vinschen wrote:
> On Oct 25 09:38, Ken Brown wrote:
>> On 10/25/2017 8:19 AM, Corinna Vinschen wrote:
>>> On Oct 25 14:11, Corinna Vinschen wrote:
>>>> Hi Ken,
>>>>
>>>> On Oct 25 07:23, Ken Brown wrote:
>>>>> Up to now the function winsup/utils/dump_setup.cc:base skips past
>>>>> colons when parsing file names.  As a result, a line like
>>>>>
>>>>>     foo foo-1:2.3-4.tar.bz2 1
>>>>>
>>>>> in /etc/setup/installed.db would cause 'cygcheck -cd foo' to report 4
>>>>> as the installed version of foo insted of 1:2.3-4.  This is not an
>>>>> issue now, but it will become an issue when version numbers are
>>>>> allowed to contain epochs.
>>>>> ---
>>>>>    winsup/utils/dump_setup.cc | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/winsup/utils/dump_setup.cc b/winsup/utils/dump_setup.cc
>>>>> index 320d69fab..3922b18f8 100644
>>>>> --- a/winsup/utils/dump_setup.cc
>>>>> +++ b/winsup/utils/dump_setup.cc
>>>>> @@ -56,7 +56,7 @@ base (const char *s)
>>>>>      const char *rv = s;
>>>>>      while (*s)
>>>>>        {
>>>>> -      if ((*s == '/' || *s == ':' || *s == '\\') && s[1])
>>>>> +      if ((*s == '/' || *s == '\\') && s[1])
>>>>
>>>> I think this is a simplified way to test for the colon in paths like
>>>> C:/foo/bar.  Nothing else makes sense in this context.
>>>>
>>>> I'm not sure how much we care, but maybe we shoulkd fix the test to
>>>> ignore the colon only if it's the second character in the incoming
>>>> string?
>>>
>>> Not "ignore", but "use as a delimiter" only as 2nd char in the input.
>>
>> I'm not sure the distinction matters in this case, since the function is
>> just trying to get the base name.  Anyway, how's the attached?
> 
> Fine, thanks.
> 
> But now that you mention it... why does parse_filename() call base() at
> all?  The filenames in installed.db are just basenames anyway.  Does
> that cover an older DB format we don't support anymore, perhaps?

It looks like parse_filename is more-or-less copied from the function 
with the same name in the setup sources (in filemanip.cc).  In that case 
there might be a good reason to call base; I'll have to look more closely.

> I just wonder now if we should simply remove base() and the call to it.
> 
> Either way, you're right, the colon check is just useless, so your first
> patch was entirely sufficient.
> 
> What do you think?  Stick to your patch or remove base()?

I vote for removing base.  The attached patch does this.

Ken
-------------- next part --------------
From f679462937faf263de682c47f8d8e73b0c7e4136 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Tue, 24 Oct 2017 18:21:53 -0400
Subject: [PATCH] winsup/utils/dump_setup.cc: Remove the function 'base'

This was called only on filenames in /etc/setup/installed.db, which
are all basenames anyway.  Moreover, base wasn't correctly handling
filenames containing colons.
---
 winsup/utils/dump_setup.cc | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/winsup/utils/dump_setup.cc b/winsup/utils/dump_setup.cc
index 320d69fab..4415954f9 100644
--- a/winsup/utils/dump_setup.cc
+++ b/winsup/utils/dump_setup.cc
@@ -48,21 +48,6 @@ find_tar_ext (const char *path)
     return 0;
 }
 
-static char *
-base (const char *s)
-{
-  if (!s)
-    return 0;
-  const char *rv = s;
-  while (*s)
-    {
-      if ((*s == '/' || *s == ':' || *s == '\\') && s[1])
-	rv = s + 1;
-      s++;
-    }
-  return (char *) rv;
-}
-
 /* Parse a filename into package, version, and extension components. */
 int
 parse_filename (const char *in_fn, fileparse& f)
@@ -79,7 +64,7 @@ parse_filename (const char *in_fn, fileparse& f)
   strcpy (f.tail, fn + n);
   fn[n] = '\0';
   f.pkg[0] = f.what[0] = '\0';
-  p = base (fn);
+  p = fn;
   for (ver = p; *ver; ver++)
     if (*ver != '-')
       continue;
-- 
2.14.2