[PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Orgad Shaneh]

From: Jon Turney <jon.turney@dronecode.org.uk>

Fix a memory leak which would occur in the case when the result of realpath() is
greater than or equal to SO_NAME_MAX_PATH_SIZE.

Distinguish between realpath() failing (returning NULL), and returning a path
longer than SO_NAME_MAX_PATH_SIZE

Warn rather than stopping with an error in those cases.

Original patch from Tim Chick.  Memory leak fix by Corinna Vinschen.
---
 gdb/windows-nat.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index a90388922e2..7dc2bb2a115 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -892,6 +892,10 @@ windows_make_so (const char *name, LPVOID load_addr)
 	{
 	  warning (_("dll path for \"%s\" too long or inaccessible"), name);
 	  so->name = so->original_name;
+	  if (rname)
+	    {
+	      free (rname);
+	    }
 	}
     }
   /* Record cygwin1.dll .text start/end.  */
-- 
2.44.0.windows.1.1.g2942425c99

[PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Orgad Shaneh]

From: Jon Turney <jon.turney@dronecode.org.uk>

Fix a memory leak which would occur in the case when the result of realpath() is
greater than or equal to SO_NAME_MAX_PATH_SIZE.

Distinguish between realpath() failing (returning NULL), and returning a path
longer than SO_NAME_MAX_PATH_SIZE

Warn rather than stopping with an error in those cases.

Original patch from Tim Chick.  Memory leak fix by Corinna Vinschen.
---
 gdb/windows-nat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index a90388922e2..29bfad5b060 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -886,13 +886,14 @@ windows_make_so (const char *name, LPVOID load_addr)
       if (rname && strlen (rname) < SO_NAME_MAX_PATH_SIZE)
 	{
 	  so->name = rname;
-	  free (rname);
 	}
       else
 	{
 	  warning (_("dll path for \"%s\" too long or inaccessible"), name);
 	  so->name = so->original_name;
 	}
+      if (rname)
+	free (rname);
     }
   /* Record cygwin1.dll .text start/end.  */
   size_t len = sizeof ("/cygwin1.dll") - 1;
-- 
2.44.0.windows.1.1.g2942425c99

Re: [PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Jon Turney]

On 21/03/2024 06:53, Orgad Shaneh wrote:
> From: Jon Turney <jon.turney@dronecode.org.uk>

Not sure where this is coming from, but this doesn't seem to be my 
latest version of this patch.

> Fix a memory leak which would occur in the case when the result of realpath() is
> greater than or equal to SO_NAME_MAX_PATH_SIZE.
> 
> Distinguish between realpath() failing (returning NULL), and returning a path
> longer than SO_NAME_MAX_PATH_SIZE
> 
> Warn rather than stopping with an error in those cases.

This line in the patch commentary, and the title, refers to the part of 
the patch submitted [1], which is already applied as commit 
a0e9b53238c3033222c53b1654da535c0743ab6e.

I separated that out because of the discussion starting at [2] ("Remove 
SO_NAME_PATH_SIZE instead"...)

[1] https://sourceware.org/pipermail/gdb-patches/2020-January/164695.html
[2] https://sourceware.org/pipermail/gdb-patches/2016-January/130435.html

Re: [PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Tom Tromey]

>>>>> "Orgad" == Orgad Shaneh <orgads@gmail.com> writes:

Orgad> From: Jon Turney <jon.turney@dronecode.org.uk>
Orgad> Fix a memory leak which would occur in the case when the result of realpath() is
Orgad> greater than or equal to SO_NAME_MAX_PATH_SIZE.

Thanks for the patch.

Orgad> +      if (rname)
Orgad> +	free (rname);

No need to check for NULL when calling free -- and gdb uses 'xfree'
anyway.

However, it would be better to change this code to use gdb_realpath,
which returns a unique pointer.  Then the memory leak will be
impossible.  Could you try that instead?

thanks,
Tom

Re: [PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Pedro Alves]

On 2024-03-21 14:45, Jon Turney wrote:
> On 21/03/2024 06:53, Orgad Shaneh wrote:
>> From: Jon Turney <jon.turney@dronecode.org.uk>
> 
> Not sure where this is coming from, but this doesn't seem to be my latest version of this patch.
> 
>> Fix a memory leak which would occur in the case when the result of realpath() is
>> greater than or equal to SO_NAME_MAX_PATH_SIZE.
>>
>> Distinguish between realpath() failing (returning NULL), and returning a path
>> longer than SO_NAME_MAX_PATH_SIZE
>>
>> Warn rather than stopping with an error in those cases.
> 
> This line in the patch commentary, and the title, refers to the part of the patch submitted [1], which is already applied as commit a0e9b53238c3033222c53b1654da535c0743ab6e.
> 
> I separated that out because of the discussion starting at [2] ("Remove SO_NAME_PATH_SIZE instead"...)
> 
> [1] https://sourceware.org/pipermail/gdb-patches/2020-January/164695.html
> [2] https://sourceware.org/pipermail/gdb-patches/2016-January/130435.html
> 

Curiously, after upstreaming your _sigbe unwinder recently, I looked at upstreaming this patch (the version of the 2016 patch 
in downstream cygwin gdb), but then this same thought of removing the limit hit me.  (at least I'm consistent over the
years, eh.)

I then realized that Simon is working on a series that switches the solib path storage to a std::string, which will
let us easily not use SO_NAME_MAX_PATH_SIZE at all in the Windows code.  So I just dropped that patch from
my upstreaming queue...

Pedro Alves

Re: [PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Orgad Shaneh]

On Thu, Mar 21, 2024 at 6:13 PM Pedro Alves <pedro@palves.net> wrote:
>
> On 2024-03-21 14:45, Jon Turney wrote:
> > On 21/03/2024 06:53, Orgad Shaneh wrote:
> >> From: Jon Turney <jon.turney@dronecode.org.uk>
> >
> > Not sure where this is coming from, but this doesn't seem to be my latest version of this patch.
> >
> >> Fix a memory leak which would occur in the case when the result of realpath() is
> >> greater than or equal to SO_NAME_MAX_PATH_SIZE.
> >>
> >> Distinguish between realpath() failing (returning NULL), and returning a path
> >> longer than SO_NAME_MAX_PATH_SIZE
> >>
> >> Warn rather than stopping with an error in those cases.
> >
> > This line in the patch commentary, and the title, refers to the part of the patch submitted [1], which is already applied as commit a0e9b53238c3033222c53b1654da535c0743ab6e.
> >
> > I separated that out because of the discussion starting at [2] ("Remove SO_NAME_PATH_SIZE instead"...)
> >
> > [1] https://sourceware.org/pipermail/gdb-patches/2020-January/164695.html
> > [2] https://sourceware.org/pipermail/gdb-patches/2016-January/130435.html

I took it from MSYS2 patches, which were taken from cygwin patches,
but apparently diverged (possibly my fault, in
https://github.com/msys2/MSYS2-packages/pull/2475. Not sure why :))

Thanks for the references. I see that some of the work is already done
(so_name, so_original_name and so->name are all std::strings), so it
looks like there's not much left.

> Curiously, after upstreaming your _sigbe unwinder recently, I looked at upstreaming this patch (the version of the 2016 patch
> in downstream cygwin gdb), but then this same thought of removing the limit hit me.  (at least I'm consistent over the
> years, eh.)
>
> I then realized that Simon is working on a series that switches the solib path storage to a std::string, which will
> let us easily not use SO_NAME_MAX_PATH_SIZE at all in the Windows code.  So I just dropped that patch from
> my upstreaming queue...

Great! So I'll drop this patch.

Thank you.

- Orgad

Re: [PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Pedro Alves]

On 2024-03-21 16:31, Orgad Shaneh wrote:
> On Thu, Mar 21, 2024 at 6:13 PM Pedro Alves <pedro@palves.net> wrote:
>>
>> On 2024-03-21 14:45, Jon Turney wrote:
>>> On 21/03/2024 06:53, Orgad Shaneh wrote:
>>>> From: Jon Turney <jon.turney@dronecode.org.uk>
>>>
>>> Not sure where this is coming from, but this doesn't seem to be my latest version of this patch.
>>>
>>>> Fix a memory leak which would occur in the case when the result of realpath() is
>>>> greater than or equal to SO_NAME_MAX_PATH_SIZE.
>>>>
>>>> Distinguish between realpath() failing (returning NULL), and returning a path
>>>> longer than SO_NAME_MAX_PATH_SIZE
>>>>
>>>> Warn rather than stopping with an error in those cases.
>>>
>>> This line in the patch commentary, and the title, refers to the part of the patch submitted [1], which is already applied as commit a0e9b53238c3033222c53b1654da535c0743ab6e.
>>>
>>> I separated that out because of the discussion starting at [2] ("Remove SO_NAME_PATH_SIZE instead"...)
>>>
>>> [1] https://sourceware.org/pipermail/gdb-patches/2020-January/164695.html
>>> [2] https://sourceware.org/pipermail/gdb-patches/2016-January/130435.html
> 
> I took it from MSYS2 patches, which were taken from cygwin patches,
> but apparently diverged (possibly my fault, in
> https://github.com/msys2/MSYS2-packages/pull/2475. Not sure why :))
> 
> Thanks for the references. I see that some of the work is already done
> (so_name, so_original_name and so->name are all std::strings), so it
> looks like there's not much left.

Indeed.  I actually thought the std::string conversion hadn't been merged yet.  Should
have checked.  :-)

So we can simplify this code already.  I sent a series to clean up things:

https://inbox.sourceware.org/gdb-patches/20240322190424.1231540-1-pedro@palves.net/T/

[PATCH 0/4] Down with SO_NAME_MAX_PATH_SIZE and windows_make_so spring cleaning
  [PATCH 1/4] Remove SO_NAME_MAX_PATH_SIZE limit from core solib code
  [PATCH 2/4] Simplify windows-nat.c:windows_make_so #ifdefery
  [PATCH 3/4] windows-nat: Remove SO_NAME_MAX_PATH_SIZE limit
  [PATCH 4/4] windows-nat: Use gdb_realpath

Thanks,
Pedro Alves

> 
>> Curiously, after upstreaming your _sigbe unwinder recently, I looked at upstreaming this patch (the version of the 2016 patch
>> in downstream cygwin gdb), but then this same thought of removing the limit hit me.  (at least I'm consistent over the
>> years, eh.)
>>
>> I then realized that Simon is working on a series that switches the solib path storage to a std::string, which will
>> let us easily not use SO_NAME_MAX_PATH_SIZE at all in the Windows code.  So I just dropped that patch from
>> my upstreaming queue...
> 
> Great! So I'll drop this patch.
> 

Re: [PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Pedro Alves]

On 03/09/2016 05:49 PM, Jon Turney wrote:
> On 20/01/2016 16:19, Pedro Alves wrote:
>> #define SO_NAME_MAX_PATH_SIZE 512       /* FIXME: Should be dynamic */
>>
>> How about just removing the limit altogether?
>>
>> Basically, make struct so_list::so_original_name and
>> struct so_list::so_name pointers instead of arrays?
> 
> I'm sorry, while that would be nice to have, that's not a project that I 
> can take on currently.
> 
> In the meantime, please consider approving this patch, or tell me what I 
> can do to make it acceptable, since it does fix an actual problem that 
> affects some users.

It's not a big change, I think.  Instead of strcpy'ing strings into
so->so_original_name and so->so_name, we'd just xfree the old
strings and xstrdup new ones.  And then in free_so, we xfree 
so->so_original_name and so->so_name.  All other code that refers to 
so->so_original_name / so->so_name should not need to change, as arrays
decay to pointers anyway.

If you want to avoid convert all targets at once, it's still quite
doable.  Add a new make_so_list() to solib.c that just does:

struct so_list *
make_so_list (void)
{
  struct so_list *new_solib = XCNEW (struct so_list);
  new_solib->so_name = xcalloc (1, SO_NAME_MAX_PATH_SIZE);
  new_solib->so_original_name = xcalloc (1, SO_NAME_MAX_PATH_SIZE);
  return new_solib;
}

and do a trivial replace of these XCNEW/XNEW by a call to make_so_list:

solib-aix.c:      struct so_list *new_solib = XCNEW (struct so_list);
solib-dsbt.c:     sop = XCNEW (struct so_list);
solib-frv.c:      sop = XCNEW (struct so_list);
solib-spu.c:              newobj = XCNEW (struct so_list);
solib-spu.c:      newobj = XCNEW (struct so_list);
solib-svr4.c:  newobj = XCNEW (struct so_list);
solib-svr4.c:  new_elem = XCNEW (struct so_list);
solib-svr4.c:  newobj = XCNEW (struct so_list);
solib-svr4.c:      newobj = XCNEW (struct so_list);
solib-target.c:      new_solib = XCNEW (struct so_list);
windows-nat.c:  so = XCNEW (struct so_list);
solib-svr4.c:      newobj = XNEW (struct so_list);

Then you can adjust the windows-nat.c solib-target.c files (the ones
Windows uses), and leave the rest to someone else (though I imagine 
converting all others would be trivial too).

Once all are converted, we can remove the initial allocations in
make_so_list.

Thanks,
Pedro Alves

Re: [PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Jon Turney]

On 20/01/2016 16:19, Pedro Alves wrote:
> #define SO_NAME_MAX_PATH_SIZE 512       /* FIXME: Should be dynamic */
>
> How about just removing the limit altogether?
>
> Basically, make struct so_list::so_original_name and
> struct so_list::so_name pointers instead of arrays?

I'm sorry, while that would be nice to have, that's not a project that I 
can take on currently.

In the meantime, please consider approving this patch, or tell me what I 
can do to make it acceptable, since it does fix an actual problem that 
affects some users.

Re: [PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Pedro Alves]

#define SO_NAME_MAX_PATH_SIZE 512       /* FIXME: Should be dynamic */

How about just removing the limit altogether?

Basically, make struct so_list::so_original_name and
struct so_list::so_name pointers instead of arrays?

Thanks,
Pedro Alves

[PATCH] Better handling for realpath() failures in windows_make_so() on Cygwin

[Jon Turney]

Fix a memory leak which would occur in the case when the result of realpath() is
greater than or equal to SO_NAME_MAX_PATH_SIZE.

Distinguish between realpath() failing (returning NULL), and returning a path
greather than or equal to SO_NAME_MAX_PATH_SIZE.

Warn rather than stopping with an error in both those cases.

Original patch from Tim Chick.  Memory leak fix by Corinna Vinschen.

See also https://cygwin.com/ml/cygwin/2015-11/msg00353.html

gdb/ChangeLog:

2016-01-20  Jon Turney  <jon.turney@dronecode.org.uk>

	* windows-nat.c (windows_make_so): Fix memory leak when realpath
	returns a path >= SO_NAME_MAX_PATH_SIZE.  Distinguish that case
	from realpath failing.  Warn rather than stopping with an error in
	both cases.
---
 gdb/ChangeLog     |  7 +++++++
 gdb/windows-nat.c | 15 ++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 71d6670..703b407 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -642,13 +642,22 @@ windows_make_so (const char *name, LPVOID load_addr)
   else
     {
       char *rname = realpath (name, NULL);
-      if (rname && strlen (rname) < SO_NAME_MAX_PATH_SIZE)
+      if (rname)
 	{
-	  strcpy (so->so_name, rname);
+	  if (strlen (rname) < SO_NAME_MAX_PATH_SIZE)
+	    strcpy (so->so_name, rname);
+	  else
+	    {
+	      warning (_("dll path \"%s\" too long"), rname);
+	      strcpy (so->so_name, so->so_original_name);
+	    }
 	  free (rname);
 	}
       else
-	error (_("dll path too long"));
+	{
+	  warning (_("dll path for \"%s\" can not be evaluated"), name);
+	  strcpy (so->so_name, so->so_original_name);
+	}
     }
   /* Record cygwin1.dll .text start/end.  */
   p = strchr (so->so_name, '\0') - (sizeof ("/cygwin1.dll") - 1);
-- 
2.7.0