Re: [RFA] Avoid calling stat with empty name in relocate_gdb_directory

Re: [RFA] Avoid calling stat with empty name in relocate_gdb_directory

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   With the troubles that I have with my patch,
Pierre> I started looking into a memory debugger...
Pierre>   I finally started to use drmemory
Pierre> for mingw compiled GDB executables.

Pierre>   I don't know if stat is supposed to handle (*dir == '\0'),
Pierre> but I thought that it should anyhow succeed in that case,
Pierre> so the patch below simply don't call stat if name is empty.

At least here I get an error.

Pierre>   Tell me if you rather think that this is a msvcrt bug that
Pierre> should not be fixed in GDB...

I don't mind working around it, but I wonder how we end up there with an
empty 'dir' in the first place.  Perhaps that is a bug instead.

Tom

RE: [RFA] Avoid calling stat with empty name in relocate_gdb_directory

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : mercredi 5 décembre 2012 19:03
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] Avoid calling stat with empty name in
> relocate_gdb_directory
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> Pierre>   With the troubles that I have with my patch,
> Pierre> I started looking into a memory debugger...
> Pierre>   I finally started to use drmemory
> Pierre> for mingw compiled GDB executables.
> 
> Pierre>   I don't know if stat is supposed to handle (*dir == '\0'),
> Pierre> but I thought that it should anyhow succeed in that case,
> Pierre> so the patch below simply don't call stat if name is empty.
> 
> At least here I get an error.
> 
> Pierre>   Tell me if you rather think that this is a msvcrt bug that
> Pierre> should not be fixed in GDB...
> 
> I don't mind working around it, but I wonder how we end up there with an
> empty 'dir' in the first place.  Perhaps that is a bug instead.

It comes from here:
388       current_directory = gdb_dirbuf;
389
390       /* Set the sysroot path.  */
391       gdb_sysroot = relocate_gdb_directory (TARGET_SYSTEM_ROOT,
392
TARGET_SYSTEM_ROOT_RELOCATABLE);
393
394       debug_file_directory = relocate_gdb_directory (DEBUGDIR,
395
DEBUGDIR_RELOCATABLE);

  TARGET_SYTEM_ROOT is set to ""
by the configure.ac autoconf script  for 

1874-# Support for --with-sysroot is a copy of GDB_AC_WITH_DIR,
1875-# except that the argument to --with-sysroot is optional.
1876-# --with-sysroot (or --with-sysroot=yes) sets the default sysroot path.
1877-if test "x$with_sysroot" = xyes; then
1878-  with_sysroot="${exec_prefix}/${target_alias}/sys-root"
1879-fi
1880-AC_ARG_WITH(sysroot,
1881-  AS_HELP_STRING([--with-sysroot@<:@=DIR@:>@],
1882-                 [search for usr/lib et al within DIR]),
1883:  [TARGET_SYSTEM_ROOT=$withval], [TARGET_SYSTEM_ROOT=])
1884:AC_DEFINE_DIR(TARGET_SYSTEM_ROOT, TARGET_SYSTEM_ROOT,
1885-              [search for usr/lib et al within DIR])
1886:AC_SUBST(TARGET_SYSTEM_ROOT)
1887:GDB_AC_DEFINE_RELOCATABLE(TARGET_SYSTEM_ROOT, sysroot,
${ac_define_dir})
1888-

  Apparently it isn't used elsewhere...
Should we modify the default value to NULL instead of ""?
I do not master complicated autoconf things like 
GDB_AC_DEFINE_RELOCATABLE well enough to know if this is
without problems...

 Tell me if you think we should modify it at configury level or
if I should commit my patch, whatever you prefer!

Pierre

Re: [RFA] Avoid calling stat with empty name in relocate_gdb_directory

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   Apparently it isn't used elsewhere...
Pierre> Should we modify the default value to NULL instead of ""?
Pierre> I do not master complicated autoconf things like 
Pierre> GDB_AC_DEFINE_RELOCATABLE well enough to know if this is
Pierre> without problems...

Pierre>  Tell me if you think we should modify it at configury level or
Pierre> if I should commit my patch, whatever you prefer!

Maybe we should just skip computing gdb_sysroot if TARGET_SYSTEM_ROOT is
"".  What do you think?

Tom

RE: [RFA] Avoid calling stat with empty name in relocate_gdb_directory

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : jeudi 6 décembre 2012 19:34
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] Avoid calling stat with empty name in
> relocate_gdb_directory
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> Pierre>   Apparently it isn't used elsewhere...
> Pierre> Should we modify the default value to NULL instead of ""?
> Pierre> I do not master complicated autoconf things like
> Pierre> GDB_AC_DEFINE_RELOCATABLE well enough to know if this is
> Pierre> without problems...
> 
> Pierre>  Tell me if you think we should modify it at configury level or
> Pierre> if I should commit my patch, whatever you prefer!
> 
> Maybe we should just skip computing gdb_sysroot if TARGET_SYSTEM_ROOT is
> "".  What do you think?

  I looked at the different calls to relocate_gdb_directory:
$ find . -iname "*.[chy]" |xargs grep -n gdb_directory
./charset.c:816:    char *iconv_dir = relocate_gdb_directory (ICONV_BIN,
./defs.h:275:extern char *relocate_gdb_directory (const char *initial, int
flag);
./jit.c:1402:  jit_reader_dir = relocate_gdb_directory (JIT_READER_DIR,
./main.c:120:relocate_gdb_directory (const char *initial, int flag)
./main.c:391:  gdb_sysroot = relocate_gdb_directory (TARGET_SYSTEM_ROOT,
./main.c:394:  debug_file_directory = relocate_gdb_directory (DEBUGDIR,
./main.c:397:  gdb_datadir = relocate_gdb_directory (GDB_DATADIR,
./main.c:405:    python_libdir = relocate_gdb_directory (tmp,
PYTHON_PATH_RELOCATABLE);


  Are we sure all other calls have non-empty first argument?
  Another problem is that currently the returned string is
always allocated, which means that we should at least
also use xstrdup ("") ...

  I still think that the approach of testing if *dir == '\0'
is safer...

  Pierre

Re: [RFA] Avoid calling stat with empty name in relocate_gdb_directory

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   I still think that the approach of testing if *dir == '\0'
Pierre> is safer...

That is ok by me.

Tom

RE: [RFA] Avoid calling stat with empty name in relocate_gdb_directory

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : jeudi 3 janvier 2013 21:59
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] Avoid calling stat with empty name in
> relocate_gdb_directory
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> Pierre>   I still think that the approach of testing if *dir == '\0'
> Pierre> is safer...
> 
> That is ok by me.
Thanks Tom,

I applied the patch.

Pierre

[RFA] Avoid calling stat with empty name in relocate_gdb_directory

[Pierre Muller]

  With the troubles that I have with my patch,
I started looking into a memory debugger...
  I finally started to use drmemory
for mingw compiled GDB executables.

  This tool reports
Error #1: UNADDRESSABLE ACCESS: reading 0x01fb3a21-0x01fb3a22 1 byte(s)
# 0 msvcrt.dll!_stat32   
# 1 relocate_gdb_directory               [../../puresrc/gdb/main.c:129]
# 2 captured_main                        [../../puresrc/gdb/main.c:391]
# 3 catch_errors
[../../puresrc/gdb/exceptions.c:546]
# 4 gdb_main                             [../../puresrc/gdb/main.c:1041]
# 5 main                                 [../../puresrc/gdb/gdb.c:34]
Note: @0:00:00.733 in thread 6596
Note: refers to 1 byte(s) beyond last valid byte in prior malloc
Note: prev lower malloc:  0x01fb3a20-0x01fb3a21
Note: instruction: cmp    0x01(%edi) $0x3a

line 129 of main.c is:
  if (stat (dir, &s) != 0 || !S_ISDIR (s.st_mode))

The unauthorized access is due to a call to stat function with dir = ""
it seems that dir[1] is access despite the fact that (dir[0]=='\0')

  I don't know if stat is supposed to handle (*dir == '\0'),
but I thought that it should anyhow succeed in that case,
so the patch below simply don't call stat if name is empty.

  Tell me if you rather think that this is a msvcrt bug that
should not be fixed in GDB...


Pierre Muller
GDB pascal language maintainer


2012-12-05  Pierre Muller  <muller@sourceware.org>

        * main.c (relocate_gdb_directory): Avoid calling stat function
        if DIR is empty.

Index: src/gdb/main.c
===================================================================
RCS file: /cvs/src/src/gdb/main.c,v
retrieving revision 1.115
diff -u -p -r1.115 main.c
--- src/gdb/main.c      16 Nov 2012 19:43:38 -0000      1.115
+++ src/gdb/main.c      5 Dec 2012 16:54:40 -0000
@@ -126,7 +126,7 @@ relocate_gdb_directory (const char *init
     {
       struct stat s;

-      if (stat (dir, &s) != 0 || !S_ISDIR (s.st_mode))
+      if (*dir == '\0' || stat (dir, &s) != 0 || !S_ISDIR (s.st_mode))
        {
          xfree (dir);
          dir = NULL;
~