[RFC] GDB crash with empty executable name (MinGW)

[RFC] GDB crash with empty executable name (MinGW)

[Joel Brobecker]

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

I happened to notice this by accident, because of a bug in our testsuite
that caused us to spawn GDB as ...

    % gdb ""

... instead of ...

    % gdb

... when we want to start GDB without an executable name.  The atypical
command where we launch GDB with an empty exec name causes the crash
on only one of our Windows machines (Win XP 32bit to be exact). To
reproduce:

    % gdb ""
    [...]
    : No such file or directory.
    (gdb) set height 0
    Critical error handler: process 2496 (c:\[...]\gdb.exe)
    terminated due to access violation

It looks like a MinGW bug - while debugging this, GDB receives a SIGTRAP
notification from ntdll:

    (gdb) step
    warning: HEAP[toto.exe]:
    warning: Heap block at 003E2460 modified at 003E2492 past requested size of 2a

    Program received signal SIGTRAP, Trace/breakpoint trap.
    0x7c91120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll

The backtrace shows that we're in _mingw_stat and that the _path has
changed from something sensible (the current working directory with a slash
at the end) to something obviously wrong:

    #8  0x0040193b in _mingw_stat (
        _path=0xffffffff <Address 0xffffffff out of bounds>, _st=0x22ff38)
        at stat.c:71

I can reproduce the same SIGTRAP debugging the following little C program,
even if that little program does not crash.

    | #include <sys/types.h>
    | #include <sys/stat.h>
    | #include <unistd.h>
    | #include <stdlib.h>
    | #include <stdio.h>
    |
    | int
    | main (void)
    | {
    |   struct stat st;
    |   const int status = stat ("c:\\[...]\\bin/", &st);
    |   void *m;
    |
    |   m = malloc (16);
    |   printf ("status = %d\n", status);
    |   free (m);
    |   return (m == NULL);
    | }

I can't confirm that this is a bug though, I haven't been able to find
the assocated file in the MinGW website (file stat.c, around line 71),
and I gave up since.  However, I think it's also reasonable to have
a short circuit that immediately returns an error if the file we are
trying to open is empty.  If anything this is a minor optimization.

2009-01-02  Joel Brobecker  <brobecker@adacore.com>

        * source.c (openp): Add assert that parameter string is not NULL.
        if parameter string is an empty string, then return with a failure
        immediately.

I have not bothered testing it yet, but I will before checking it in,
if there are no objections.  Given how rare it must be to call GDB with
an empty executable name, I do not think that this is a very critical patch.

Anyone in favor?
-- 
Joel

[-- Attachment #2: 0001-GDB-crash-with-empty-executable-name.patch --]
[-- Type: text/x-diff, Size: 971 bytes --]

From fa66afbd4e3ec9abd384ff7b2c754e4d545079e2 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Sat, 2 Jan 2010 13:44:04 +0100
Subject: [PATCH] GDB crash with empty executable name.

        * source.c (openp): Add assert that parameter string is not NULL.
        if parameter string is an empty string, then return with a failure
        immediately.

---
 gdb/source.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/gdb/source.c b/gdb/source.c
index 661bbd6..9f8376a 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -707,6 +707,14 @@ openp (const char *path, int opts, const char *string,
 
   /* The open syscall MODE parameter is not specified.  */
   gdb_assert ((mode & O_CREAT) == 0);
+  gdb_assert (string != NULL);
+
+  /* A file with an empty name cannot possibly exist.  */
+  if (string[0] == '\0')
+    {
+      errno = ENOENT;
+      return -1;
+    }
 
   if (!path)
     path = ".";
-- 
1.5.5.1

Re: [RFC] GDB crash with empty executable name (MinGW)

[Christopher Faylor]

On Sat, Jan 02, 2010 at 04:46:22PM +0400, Joel Brobecker wrote:
>I happened to notice this by accident, because of a bug in our testsuite
>that caused us to spawn GDB as ...
>
>    % gdb ""
>
>... instead of ...
>
>    % gdb
>
>... when we want to start GDB without an executable name.  The atypical
>command where we launch GDB with an empty exec name causes the crash
>on only one of our Windows machines (Win XP 32bit to be exact). To
>reproduce:
>
>    % gdb ""
>    [...]
>    : No such file or directory.
>    (gdb) set height 0
>    Critical error handler: process 2496 (c:\[...]\gdb.exe)
>    terminated due to access violation
>
>It looks like a MinGW bug - while debugging this, GDB receives a SIGTRAP
>notification from ntdll:
>
>    (gdb) step
>    warning: HEAP[toto.exe]:
>    warning: Heap block at 003E2460 modified at 003E2492 past requested size of 2a
>
>    Program received signal SIGTRAP, Trace/breakpoint trap.
>    0x7c91120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
>
>The backtrace shows that we're in _mingw_stat and that the _path has
>changed from something sensible (the current working directory with a slash
>at the end) to something obviously wrong:
>
>    #8  0x0040193b in _mingw_stat (
>        _path=0xffffffff <Address 0xffffffff out of bounds>, _st=0x22ff38)
>        at stat.c:71
>
>I can reproduce the same SIGTRAP debugging the following little C program,
>even if that little program does not crash.
>
>    | #include <sys/types.h>
>    | #include <sys/stat.h>
>    | #include <unistd.h>
>    | #include <stdlib.h>
>    | #include <stdio.h>
>    |
>    | int
>    | main (void)
>    | {
>    |   struct stat st;
>    |   const int status = stat ("c:\\[...]\\bin/", &st);
>    |   void *m;
>    |
>    |   m = malloc (16);
>    |   printf ("status = %d\n", status);
>    |   free (m);
>    |   return (m == NULL);
>    | }
>
>I can't confirm that this is a bug though, I haven't been able to find
>the assocated file in the MinGW website (file stat.c, around line 71),
>and I gave up since.  However, I think it's also reasonable to have
>a short circuit that immediately returns an error if the file we are
>trying to open is empty.  If anything this is a minor optimization.
>
>2009-01-02  Joel Brobecker  <brobecker@adacore.com>
>
>        * source.c (openp): Add assert that parameter string is not NULL.
>        if parameter string is an empty string, then return with a failure
>        immediately.
>
>I have not bothered testing it yet, but I will before checking it in,
>if there are no objections.  Given how rare it must be to call GDB with
>an empty executable name, I do not think that this is a very critical patch.
>
>Anyone in favor?

Given that this is apparently a mingw (or Windows???) bug your fix makes
sense to me.

cgf

Re: [RFC] GDB crash with empty executable name (MinGW)

[Michael]

I don't have my illegal copy of windows present at the moment, I'm so 
sorry I can't help you :(

Hope we can get the boost mailing-list and the gdb mailing-list on the 
same level.

Greetz,
Michael

Joel Brobecker wrote:
> I happened to notice this by accident, because of a bug in our testsuite
> that caused us to spawn GDB as ...
>
>     % gdb ""
>
> ... instead of ...
>
>     % gdb
>
> ... when we want to start GDB without an executable name.  The atypical
> command where we launch GDB with an empty exec name causes the crash
> on only one of our Windows machines (Win XP 32bit to be exact). To
> reproduce:
>
>     % gdb ""
>     [...]
>     : No such file or directory.
>     (gdb) set height 0
>     Critical error handler: process 2496 (c:\[...]\gdb.exe)
>     terminated due to access violation
>
> It looks like a MinGW bug - while debugging this, GDB receives a SIGTRAP
> notification from ntdll:
>
>     (gdb) step
>     warning: HEAP[toto.exe]:
>     warning: Heap block at 003E2460 modified at 003E2492 past requested size of 2a
>
>     Program received signal SIGTRAP, Trace/breakpoint trap.
>     0x7c91120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
>
> The backtrace shows that we're in _mingw_stat and that the _path has
> changed from something sensible (the current working directory with a slash
> at the end) to something obviously wrong:
>
>     #8  0x0040193b in _mingw_stat (
>         _path=0xffffffff <Address 0xffffffff out of bounds>, _st=0x22ff38)
>         at stat.c:71
>
> I can reproduce the same SIGTRAP debugging the following little C program,
> even if that little program does not crash.
>
>     | #include <sys/types.h>
>     | #include <sys/stat.h>
>     | #include <unistd.h>
>     | #include <stdlib.h>
>     | #include <stdio.h>
>     |
>     | int
>     | main (void)
>     | {
>     |   struct stat st;
>     |   const int status = stat ("c:\\[...]\\bin/", &st);
>     |   void *m;
>     |
>     |   m = malloc (16);
>     |   printf ("status = %d\n", status);
>     |   free (m);
>     |   return (m == NULL);
>     | }
>
> I can't confirm that this is a bug though, I haven't been able to find
> the assocated file in the MinGW website (file stat.c, around line 71),
> and I gave up since.  However, I think it's also reasonable to have
> a short circuit that immediately returns an error if the file we are
> trying to open is empty.  If anything this is a minor optimization.
>
> 2009-01-02  Joel Brobecker  <brobecker@adacore.com>
>
>         * source.c (openp): Add assert that parameter string is not NULL.
>         if parameter string is an empty string, then return with a failure
>         immediately.
>
> I have not bothered testing it yet, but I will before checking it in,
> if there are no objections.  Given how rare it must be to call GDB with
> an empty executable name, I do not think that this is a very critical patch.
>
> Anyone in favor?
>   

Re: [RFC] GDB crash with empty executable name (MinGW)

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> 2009-01-02  Joel Brobecker  <brobecker@adacore.com>
Joel>         * source.c (openp): Add assert that parameter string is not NULL.
Joel>         if parameter string is an empty string, then return with a failure
Joel>         immediately.

Joel> Anyone in favor?

I think it would be fine, but I would recommend putting some info into
the comment explaining why this check was added... just some detail from
your original note.

Tom

Re: [RFC] GDB crash with empty executable name (MinGW)

[Joel Brobecker]

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

> I think it would be fine, but I would recommend putting some info into
> the comment explaining why this check was added... just some detail from
> your original note.

Agreed. Here is what I checked in. I can make additional adjustments
if necessary.

Thank you!
-- 
Joel

[-- Attachment #2: source.diff --]
[-- Type: text/x-diff, Size: 1182 bytes --]

commit f15ba96bbf48880f5a60443e78bab39070a5fc33
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Fri Jan 8 16:15:54 2010 +0400

    GDB crash with empty executable name (MinGW).
    
            * source.c (openp): Add assert that parameter string is not NULL.
            if parameter string is an empty string, then return with a failure
            immediately.

diff --git a/gdb/source.c b/gdb/source.c
index fcfce65..2090326 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -707,6 +707,20 @@ openp (const char *path, int opts, const char *string,
 
   /* The open syscall MODE parameter is not specified.  */
   gdb_assert ((mode & O_CREAT) == 0);
+  gdb_assert (string != NULL);
+
+  /* A file with an empty name cannot possibly exist.  Report a failure
+     without further checking.
+
+     This is an optimization which also defends us against buggy
+     implementations of the "stat" function.  For instance, we have
+     noticed that a MinGW debugger built on Windows XP 32bits crashes
+     when the debugger is started with an empty argument.  */
+  if (string[0] == '\0')
+    {
+      errno = ENOENT;
+      return -1;
+    }
 
   if (!path)
     path = ".";