[review] Use safe_strerror in some Windows code
Commit Message
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
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)
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.
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!
@@ -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,
@@ -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.
@@ -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;
@@ -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
}