mkshortcut (cygutils-1.4.14) free error

mkshortcut (cygutils-1.4.14) free error

[Anthony Heading]

Seems free() is being called on an adjusted pointer,  causing general
misbehaviour.  A minimal patch below.

Anthony


--- src/mkshortcut/mkshortcut.c.Orig    2015-10-17 21:57:08.000000000
-0400
+++ src/mkshortcut/mkshortcut.c 2015-10-17 22:16:41.269357400 -0400
@@ -412,7 +412,7 @@
   char* exe_name = NULL;
   char* dir_name = NULL;
   char* desc = NULL;
-  char *buf_str, *tmp_str;
+  char *buf_str, *tmp_str, *base_str;
   int tmp;
 
   /* For OLE interface */
@@ -489,6 +489,7 @@
     {
       /*  Strip trailing /'s if any */
       buf_str = xstrndup (opts.target_arg, strlen(opts.target_arg));
+      base_str = buf_str;
       tmp_str = buf_str;
       tmp = strlen (buf_str) - 1;
       while (strrchr (buf_str, '/') == (buf_str + tmp))
@@ -504,7 +505,7 @@
           buf_str++;
         }
       link_name = xstrndup (tmp_str, strlen (tmp_str));
-      free (buf_str);
+      free (base_str);
     }
   /*  User specified a name, so check it and convert  */
   else

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Anthony Heading]

Hi,
I see the cygutils package has been orphaned, perhaps explaining the
lack of response to the below.  
A related question then is that the vanilla mkshortcut built from the
latest source package does not
seem to perfectly match the behaviour of the binary.    Is there any way
to review or reproduce how
the binary package was built, or any other way to verify that the source
and binaries do match? 
Thanks
Anthony


On Sat, Oct 17, 2015, at 10:30 PM, Anthony Heading wrote:
> Seems free() is being called on an adjusted pointer,  causing general
> misbehaviour.  A minimal patch below.
> 
> Anthony
> 
> 
> --- src/mkshortcut/mkshortcut.c.Orig    2015-10-17 21:57:08.000000000
> -0400
> +++ src/mkshortcut/mkshortcut.c 2015-10-17 22:16:41.269357400 -0400
> @@ -412,7 +412,7 @@
>    char* exe_name = NULL;
>    char* dir_name = NULL;
>    char* desc = NULL;
> -  char *buf_str, *tmp_str;
> +  char *buf_str, *tmp_str, *base_str;
>    int tmp;
>  
>    /* For OLE interface */
> @@ -489,6 +489,7 @@
>      {
>        /*  Strip trailing /'s if any */
>        buf_str = xstrndup (opts.target_arg, strlen(opts.target_arg));
> +      base_str = buf_str;
>        tmp_str = buf_str;
>        tmp = strlen (buf_str) - 1;
>        while (strrchr (buf_str, '/') == (buf_str + tmp))
> @@ -504,7 +505,7 @@
>            buf_str++;
>          }
>        link_name = xstrndup (tmp_str, strlen (tmp_str));
> -      free (buf_str);
> +      free (base_str);
>      }
>    /*  User specified a name, so check it and convert  */
>    else
> 
> --
> Problem reports:       http://cygwin.com/problems.html
> FAQ:                   http://cygwin.com/faq/
> Documentation:         http://cygwin.com/docs.html
> Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
> 

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Mark Geisert]

Anthony Heading writes:
> I see the cygutils package has been orphaned, perhaps explaining the
> lack of response to the below.  
> A related question then is that the vanilla mkshortcut built from the
> latest source package does not
> seem to perfectly match the behaviour of the binary.    Is there any way
> to review or reproduce how
> the binary package was built, or any other way to verify that the source
> and binaries do match?

Unfortunately I have no answers for your good questions above.  But...

> On Sat, Oct 17, 2015, at 10:30 PM, Anthony Heading wrote:
> > Seems free() is being called on an adjusted pointer,  causing general
> > misbehaviour.  A minimal patch below.

[...sorry for deletions within the patch; trying to satisfy gmane...]
> > -  char *buf_str, *tmp_str;
> > +  char *buf_str, *tmp_str, *base_str;
[...]
> >        buf_str = xstrndup (opts.target_arg, strlen(opts.target_arg));
> > +      base_str = buf_str;
> >        tmp_str = buf_str;

...if tmp_str is not modified after the above line, ...

> >        tmp = strlen (buf_str) - 1;
> >        while (strrchr (buf_str, '/') == (buf_str + tmp))
> >  <at>  <at>  -504,7 +505,7  <at>  <at> 
> >            buf_str++;
> >          }
> >        link_name = xstrndup (tmp_str, strlen (tmp_str));
> > -      free (buf_str);
> > +      free (base_str);

...then "free(tmp_str);" should solve the original issue without having to
define a new variable base_str.  I didn't look at the complete original
code, only the patch that you posted, but hopefully this points to a "more
minimal" fix if you agree it's solid.

..mark




--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1991 bytes --]

On Oct 26 03:57, Mark Geisert wrote:
> Anthony Heading writes:
> > I see the cygutils package has been orphaned, perhaps explaining the
> > lack of response to the below.  
> > A related question then is that the vanilla mkshortcut built from the
> > latest source package does not
> > seem to perfectly match the behaviour of the binary.    Is there any way
> > to review or reproduce how
> > the binary package was built, or any other way to verify that the source
> > and binaries do match?
> 
> Unfortunately I have no answers for your good questions above.  But...
> 
> > On Sat, Oct 17, 2015, at 10:30 PM, Anthony Heading wrote:
> > > Seems free() is being called on an adjusted pointer,  causing general
> > > misbehaviour.  A minimal patch below.
> 
> [...sorry for deletions within the patch; trying to satisfy gmane...]
> > > -  char *buf_str, *tmp_str;
> > > +  char *buf_str, *tmp_str, *base_str;
> [...]
> > >        buf_str = xstrndup (opts.target_arg, strlen(opts.target_arg));
> > > +      base_str = buf_str;
> > >        tmp_str = buf_str;
> 
> ...if tmp_str is not modified after the above line, ...
> 
> > >        tmp = strlen (buf_str) - 1;
> > >        while (strrchr (buf_str, '/') == (buf_str + tmp))
> > >  <at>  <at>  -504,7 +505,7  <at>  <at> 
> > >            buf_str++;
> > >          }
> > >        link_name = xstrndup (tmp_str, strlen (tmp_str));
> > > -      free (buf_str);
> > > +      free (base_str);
> 
> ...then "free(tmp_str);" should solve the original issue without having to
> define a new variable base_str.  I didn't look at the complete original
> code, only the patch that you posted, but hopefully this points to a "more
> minimal" fix if you agree it's solid.

Ideally somebody takes a heart and takes over the package as maintainer :}


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: mkshortcut (cygutils-1.4.14) free error

[Ken Brown]

On 10/25/2015 9:45 PM, Anthony Heading wrote:
> Hi,
> I see the cygutils package has been orphaned, perhaps explaining the
> lack of response to the below.
> A related question then is that the vanilla mkshortcut built from the
> latest source package does not
> seem to perfectly match the behaviour of the binary.    Is there any way
> to review or reproduce how
> the binary package was built,

To see how it was built, download the source using setup-*.exe and look 
at the file /usr/src/cygutils.cygport.  To reproduce the build, install 
the cygport package and run 'cygport cygutils.cygport all'.  To fix the 
problem, volunteer to be the cygutils maintainer.

Ken


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Mark Geisert]

Ken Brown writes:
> Anthony Heading wrote:
> > Hi,
> > I see the cygutils package has been orphaned, perhaps explaining the
> > lack of response to the below.
> > A related question then is that the vanilla mkshortcut built from the
> > latest source package does not
> > seem to perfectly match the behaviour of the binary.    Is there any way
> > to review or reproduce how
> > the binary package was built,
> 
> To see how it was built, download the source using setup-*.exe and look 
> at the file /usr/src/cygutils.cygport.  To reproduce the build, install 
> the cygport package and run 'cygport cygutils.cygport all'.  To fix the 
> problem, volunteer to be the cygutils maintainer.

There are configure errors from this process, at least on my machine.  I'll
take on maintenance of this package.  Give me a short while to get my sea
legs.  Thanks Ken for the pointers and Corinna for the nudge.

..mark



--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Anthony Heading]

On Sun, Oct 25, 2015, at 11:57 PM, Mark Geisert wrote:
> ...if tmp_str is not modified after the above line, ...

Indeed.   Unfortunately it is modified.

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Anthony Heading]

On Mon, Oct 26, 2015, at 03:54 PM, Mark Geisert wrote:
> There are configure errors from this process, at least on my
> machine. I'll take on maintenance of this package.  Give me a short
> while to get my sea legs.  Thanks Ken for the pointers and Corinna
> for the nudge.

Thanks Mark.   I would try to suggest you beat me to the punch but the
truth is you're much more noble.

I didn't hit any explicit configure issue myself,  rather a bunch of
unrecognized case-invariant strcmp variants, which took about 10 seconds
to fix but did make me wonder about the source code.    And then this
below was the slightly trickier issue:

% src/mkshortcut/.libs/mkshortcut xyzzy
mkshortcut: Saving "xyzzy.lnk" failed; does the target directory exist?

The packaged binary does not do this.

Fiddling about with various command-line flags suggests the issue is
with saving the link to a non-absolute path;  Google sort of vaguely
implies that IPersistFile may have changed semantics starting in Windows
8, but the docs I have say the Save() path should be absolute.   So I
added the minimal patch below.   At least, I think it's minimal, but I'd
certainly defer to you as the new maintainer!    Anyhow, attached below
fwiw in case it's of any help.

Rgds

Anthony

--- mkshortcut.c.orig   2015-10-17 21:57:08.000000000 -0400
+++ mkshortcut.c        2015-10-17 23:52:50.723030500 -0400
@@ -421,6 +421,7 @@
   IShellLink *shell_link;
   IPersistFile *persist_file;
   WCHAR widepath[MAX_PATH];
+  char link_path[MAX_PATH];
 
   /*  If there's a colon in the TARGET, it should be a URL */
   if (strchr (opts.target_arg, ':') != NULL)
@@ -596,6 +598,14 @@
       free (buf_str);
     }
 
+  hres = GetFullPathName(link_name, sizeof(link_path), link_path, 0);
+  if (hres == 0) {
+      fprintf (stderr, "%s: Could not qualify link name\n",
program_name);
+      return 2;
+  }
+  free (link_name);
+  link_name = xstrndup(link_path, strlen(link_path));
+
   /* Setup description text */
   if (opts.desc_arg != NULL)
     {

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Mark Geisert]

Anthony Heading writes:
> I didn't hit any explicit configure issue myself,  rather a bunch of
> unrecognized case-invariant strcmp variants, which took about 10 seconds
> to fix but did make me wonder about the source code.    And then this
> below was the slightly trickier issue:
> 
> % src/mkshortcut/.libs/mkshortcut xyzzy
> mkshortcut: Saving "xyzzy.lnk" failed; does the target directory exist?
> 
> The packaged binary does not do this.

I really appreciate the leads and code you've provided.  Could we please
discuss only on the Cygwin mailing list?  That's the convention we have,
barring extraordinary circumstances :) .  It allows for public review.

Installing gettext-devel and popt-devel got rid of my configure issues.  I
updated lpr/Printer.cc and cygdrop/cygdrop.cc to use strcasecmp() instead of
stricmp() and strcmpi() respectively.  I did not (yet) look into the
compiler warnings that spew during the build of some of the cygutils
executables.

At that point I could reproduce your first mkshortcut issue.  Your patch
seems to fix that issue.  So far, so good.

The second issue with the non-absolute path is more problematic.  Without
your second patch, I do see the issue but only on the 2nd or later
invocation.  In other words, if the xyzzy.lnk file does not initially exist,
the command 'src/mkshortcut/.libs/mkshortcut xyzzy' works and does create
the link file.  Another invocation then shows the error.  Is it simply
mis-reporting that there's an existing link file?  I'm also assuming you're
in the /usr/src/cygutils-1.4.14-1.<arch>/build directory when you issue that
command.

..mark


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Anthony Heading]

On Tue, Oct 27, 2015, at 01:29 AM, Mark Geisert wrote:
> I really appreciate the leads and code you've provided. Could we
> please discuss only on the Cygwin mailing list? That's the convention
> we have, barring extraordinary circumstances :) . It allows for
> public review.

I believe I did send everything to the mailing list. I may have copied
you directly also. Apologies if that was inconvenient.

> At that point I could reproduce your first mkshortcut issue. Your
> patch seems to fix that issue. So far, so good.

Great.

> The second issue with the non-absolute path is more problematic.
> Without your second patch, I do see the issue but only on the 2nd or
> later invocation. In other words, if the xyzzy.lnk file does not
> initially exist, the command 'src/mkshortcut/.libs/mkshortcut xyzzy'
> works and does create the link file. Another invocation then shows the
> error. Is it simply mis-reporting that there's an existing link file?

Yes. The hint in the error message is unhelpful I think, since a missing
directory is only one of a myriad of possible errors. As you note, an
existing link prompts the same message.

On the build I made on Windows 10, however, I hit the problem on the
very first invocation. I imagine you therefore are building on Windows 7
or earlier, which seems to produce binaries that work OK.  As I said, I
suspect this is because IPersistFile::Save has changed semantics, e.g.
per this link:

https://msdn.microsoft.com/en-us/library/windows/desktop/hh848036%28v=vs.85%29.aspx

I haven't verified this;  I don't know how why or whether gcc is
volunteering the executable to run in Windows 8 mode, or if there's a
manifest in some dll library stub, or whether the COM ID of the
IPersistFile interface is redirected by newer headers, or any similar
variant of Windows black magic,  but it seems to me like a good guess.

If you're going to stay building on Windows 7 then, I don't think this
latter patch is needed,  although it's hopefully harmless.  But I guess
without it the code will silently break when Windows 8 or newer comes
into the picture.

Regards

Anthony

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Mark Geisert]

Anthony Heading writes:
> > The second issue with the non-absolute path is more problematic.
> > Without your second patch, I do see the issue but only on the 2nd or
> > later invocation. In other words, if the xyzzy.lnk file does not
> > initially exist, the command 'src/mkshortcut/.libs/mkshortcut xyzzy'
> > works and does create the link file. Another invocation then shows the
> > error. Is it simply mis-reporting that there's an existing link file?
> 
> Yes. The hint in the error message is unhelpful I think, since a missing
> directory is only one of a myriad of possible errors. As you note, an
> existing link prompts the same message.

Re the unhelpful hint, I was considering removing the guess it's making or
substituting something more generic.  I'm not sure it would help.  If
there were some way to get the underlying Windows error code we might be
able to map it to an errno but that seems like it might not be robust.  If
we only have OLE/COM error codes like the "E_FAIL" for an existing link
file, it seems hopeless.  I'll probably leave the error message text as-is
for now.
 
> On the build I made on Windows 10, however, I hit the problem on the
> very first invocation. I imagine you therefore are building on Windows 7
> or earlier, which seems to produce binaries that work OK.

Yes, I'm building on Windows 7, 64- and 32-bit as well as Windows XP.  No
plans at present to upgrade but I might need to consider VMs to test later
Windows versions.

> As I said, I
> suspect this is because IPersistFile::Save has changed semantics, e.g.
> per this link:
>
https://msdn.microsoft.com/en-us/library/
windows/desktop/hh848036%28v=vs.85%29.aspx
> 
> I haven't verified this;  I don't know how why or whether gcc is
> volunteering the executable to run in Windows 8 mode, or if there's a
> manifest in some dll library stub, or whether the COM ID of the
> IPersistFile interface is redirected by newer headers, or any similar
> variant of Windows black magic,  but it seems to me like a good guess.

I'll happily defer to your understanding of Those Black Arts :).
 
> If you're going to stay building on Windows 7 then, I don't think this
> latter patch is needed,  although it's hopefully harmless.  But I guess
> without it the code will silently break when Windows 8 or newer comes
> into the picture.

I think we should take advantage of the detective work you've done and
patch as you've suggested.  I'll add a comment to the effect that it's for
a specific Windows behavior change seen after Windows 7.  I'll add a link
to that MSDN page if our coding/doc conventions allow for that.

Thanks again for your patient and detailed help on this!

..mark




--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: mkshortcut (cygutils-1.4.14) free error

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On Oct 28 04:49, Mark Geisert wrote:
> Anthony Heading writes:
> > On the build I made on Windows 10, however, I hit the problem on the
> > very first invocation. I imagine you therefore are building on Windows 7
> > or earlier, which seems to produce binaries that work OK.
> 
> Yes, I'm building on Windows 7, 64- and 32-bit as well as Windows XP.

Uh oh, you know the vultures are circling around XPs head...


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: mkshortcut (cygutils-1.4.14) free error

[Mark Geisert]

Corinna Vinschen writes:
> On Oct 28 04:49, Mark Geisert wrote:
> > Anthony Heading writes:
> > > On the build I made on Windows 10, however, I hit the problem on the
> > > very first invocation. I imagine you therefore are building on Windows 7
> > > or earlier, which seems to produce binaries that work OK.
> > 
> > Yes, I'm building on Windows 7, 64- and 32-bit as well as Windows XP.
> 
> Uh oh, you know the vultures are circling around XPs head...

Indeed.  I only meant I have no current plans to upgrade beyond Win7.  That
XP machine is way overdue for its upgrade to Win7.

..mark




--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple