diff mbox

[review] Use safe_strerror in some Windows code

Message ID gerrit.1572562283000.I10ef21efcac72628e32f5eac4cc22174c11c2d39@gnutoolchain-gerrit.osci.io
State New
Headers show

Commit Message

Simon Marchi (Code Review) Oct. 31, 2019, 10:51 p.m. UTC
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477
......................................................................

Use safe_strerror in some Windows code

I noticed that win32-low.c had its own wrapper for FormatMessage, and
that gdb_dlopen also called FormatMessage (and leaked the buffer).

This patch removes strwinerror in favor of safe_strerror, and changes
gdb_dlopen to use that as well.

gdb/ChangeLog
2019-10-31  Tom Tromey  <tromey@adacore.com>

	* gdbsupport/gdb-dlfcn.c (gdb_dlopen): Use safe_strerror.

gdb/gdbserver/ChangeLog
2019-10-31  Tom Tromey  <tromey@adacore.com>

	* win32-low.c (strwinerror): Remove.
	(win32_require_context, continue_one_thread)
	(win32_create_inferior, win32_attach, suspend_one_thread): Use
	safe_strerror.

Change-Id: I10ef21efcac72628e32f5eac4cc22174c11c2d39
---
M gdb/ChangeLog
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/win32-low.c
M gdb/gdbsupport/gdb-dlfcn.c
4 files changed, 18 insertions(+), 72 deletions(-)

Comments

Simon Marchi (Code Review) Oct. 31, 2019, 11:18 p.m. UTC | #1
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477/1/gdb/gdbserver/win32-low.c 
File gdb/gdbserver/win32-low.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477/1/gdb/gdbserver/win32-low.c@a513 
PS1, Line 513: 
508 | 
509 | char *
510 | strwinerror (DWORD error)
511 | {
512 |   static char buf[1024];
513 >   TCHAR *msgbuf;
514 |   DWORD lasterr = GetLastError ();
515 |   DWORD chars = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
516 | 			       | FORMAT_MESSAGE_ALLOCATE_BUFFER,
517 | 			       NULL,
518 | 			       error,

However, this function could be improved by just using char and FormatMessageA instead of potentially calling the wide version for no real reason.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477/1/gdb/gdbserver/win32-low.c@181 
PS1, Line 181: 
171 | win32_require_context (win32_thread_info *th)
    | ...
176 | 	{
177 | 	  if (SuspendThread (th->h) == (DWORD) -1)
178 | 	    {
179 | 	      DWORD err = GetLastError ();
180 | 	      OUTMSG (("warning: SuspendThread failed in thread_rec, "
181 > 		       "(error %d): %s\n", (int) err, safe_strerror (err)));
182 | 	    }
183 | 	  else
184 | 	    th->suspended = 1;
185 | 	}
186 | 

Hm is strerror compatible with GetLastError (vs errno)? I'm almost certain that is not the case? (an issue throughout the patch)
Simon Marchi (Code Review) Nov. 1, 2019, 1:43 p.m. UTC | #2
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477/1/gdb/gdbserver/win32-low.c 
File gdb/gdbserver/win32-low.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477/1/gdb/gdbserver/win32-low.c@181 
PS1, Line 181: 
171 | win32_require_context (win32_thread_info *th)
    | ...
176 | 	{
177 | 	  if (SuspendThread (th->h) == (DWORD) -1)
178 | 	    {
179 | 	      DWORD err = GetLastError ();
180 | 	      OUTMSG (("warning: SuspendThread failed in thread_rec, "
181 > 		       "(error %d): %s\n", (int) err, safe_strerror (err)));
182 | 	    }
183 | 	  else
184 | 	    th->suspended = 1;
185 | 	}
186 | 

> Hm is strerror compatible with GetLastError (vs errno)? I'm almost certain that is not the case? (an issue throughout the patch)

Yeah, I think this patch is super wrong.  Also it's incomplete.
I'm going to redo it a different way... I'll abandon this one and
land the other two.
Simon Marchi (Code Review) Nov. 1, 2019, 1:43 p.m. UTC | #3
Tom Tromey has abandoned this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/477 )

Change subject: Use safe_strerror in some Windows code
......................................................................


Abandoned

Basically incorrect!
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3c26e6d..245fcdf 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@ 
+2019-10-31  Tom Tromey  <tromey@adacore.com>
+
+	* gdbsupport/gdb-dlfcn.c (gdb_dlopen): Use safe_strerror.
+
 2019-10-31  Christian Biesinger  <cbiesinger@google.com>
 
 	* agent.c (set_can_use_agent): When the setting is turned on,
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index ab03f88..99189cd 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,10 @@ 
+2019-10-31  Tom Tromey  <tromey@adacore.com>
+
+	* win32-low.c (strwinerror): Remove.
+	(win32_require_context, continue_one_thread)
+	(win32_create_inferior, win32_attach, suspend_one_thread): Use
+	safe_strerror.
+
 2019-10-31  Christian Biesinger  <cbiesinger@google.com>
 
 	* config.in: Regenerate.
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 449ed5f..bdf2ecf 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -178,7 +178,7 @@ 
 	    {
 	      DWORD err = GetLastError ();
 	      OUTMSG (("warning: SuspendThread failed in thread_rec, "
-		       "(error %d): %s\n", (int) err, strwinerror (err)));
+		       "(error %d): %s\n", (int) err, safe_strerror (err)));
 	    }
 	  else
 	    th->suspended = 1;
@@ -443,7 +443,7 @@ 
 	    {
 	      DWORD err = GetLastError ();
 	      OUTMSG (("warning: ResumeThread failed in continue_one_thread, "
-		       "(error %d): %s\n", (int) err, strwinerror (err)));
+		       "(error %d): %s\n", (int) err, safe_strerror (err)));
 	    }
 	  th->suspended = 0;
 	}
@@ -496,61 +496,6 @@ 
       (*the_low_target.store_inferior_register) (regcache, th, regno);
 }
 
-/* Map the Windows error number in ERROR to a locale-dependent error
-   message string and return a pointer to it.  Typically, the values
-   for ERROR come from GetLastError.
-
-   The string pointed to shall not be modified by the application,
-   but may be overwritten by a subsequent call to strwinerror
-
-   The strwinerror function does not change the current setting
-   of GetLastError.  */
-
-char *
-strwinerror (DWORD error)
-{
-  static char buf[1024];
-  TCHAR *msgbuf;
-  DWORD lasterr = GetLastError ();
-  DWORD chars = FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM
-			       | FORMAT_MESSAGE_ALLOCATE_BUFFER,
-			       NULL,
-			       error,
-			       0, /* Default language */
-			       (LPTSTR) &msgbuf,
-			       0,
-			       NULL);
-  if (chars != 0)
-    {
-      /* If there is an \r\n appended, zap it.  */
-      if (chars >= 2
-	  && msgbuf[chars - 2] == '\r'
-	  && msgbuf[chars - 1] == '\n')
-	{
-	  chars -= 2;
-	  msgbuf[chars] = 0;
-	}
-
-      if (chars > ((COUNTOF (buf)) - 1))
-	{
-	  chars = COUNTOF (buf) - 1;
-	  msgbuf [chars] = 0;
-	}
-
-#ifdef UNICODE
-      wcstombs (buf, msgbuf, chars + 1);
-#else
-      strncpy (buf, msgbuf, chars + 1);
-#endif
-      LocalFree (msgbuf);
-    }
-  else
-    sprintf (buf, "unknown win32 error (%u)", (unsigned) error);
-
-  SetLastError (lasterr);
-  return buf;
-}
-
 static BOOL
 create_process (const char *program, char *args,
 		DWORD flags, PROCESS_INFORMATION *pi)
@@ -687,7 +632,7 @@ 
   if (!ret)
     {
       error ("Error creating process \"%s%s\", (error %d): %s\n",
-	     program, args, (int) err, strwinerror (err));
+	     program, args, (int) err, safe_strerror (err));
     }
   else
     {
@@ -745,7 +690,7 @@ 
 
   err = GetLastError ();
   error ("Attach to process failed (error %d): %s\n",
-	 (int) err, strwinerror (err));
+	 (int) err, safe_strerror (err));
 }
 
 /* Handle OUTPUT_DEBUG_STRING_EVENT from child process.  */
@@ -1343,7 +1288,7 @@ 
 	{
 	  DWORD err = GetLastError ();
 	  OUTMSG (("warning: SuspendThread failed in suspend_one_thread, "
-		   "(error %d): %s\n", (int) err, strwinerror (err)));
+		   "(error %d): %s\n", (int) err, safe_strerror (err)));
 	}
       else
 	th->suspended = 1;
diff --git a/gdb/gdbsupport/gdb-dlfcn.c b/gdb/gdbsupport/gdb-dlfcn.c
index 921f10f..12a35fe 100644
--- a/gdb/gdbsupport/gdb-dlfcn.c
+++ b/gdb/gdbsupport/gdb-dlfcn.c
@@ -73,18 +73,8 @@ 
   error (_("Could not load %s: %s"), filename, dlerror());
 #else
   {
-    LPVOID buffer;
-    DWORD dw;
-
-    dw = GetLastError();
-
-    FormatMessage (FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM |
-                   FORMAT_MESSAGE_IGNORE_INSERTS,
-                   NULL, dw, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
-                   (LPTSTR) &buffer,
-                   0, NULL);
-
-    error (_("Could not load %s: %s"), filename, (char *) buffer);
+    char *msg = safe_strerror (GetLastError ());
+    error (_("Could not load %s: %s"), filename, msg);
   }
 #endif
 }