gdb/source: Fix open_source_file error handling
Commit Message
open_source_file relies on errno to communicate the reason for a missing
source file.
open_source_file also may call debuginfod_find_source. It is possible
for debuginfod_find_source to set errno to a value unrelated to the
reason for a failed download.
This can result in bogus error messages being reported as the reason for
a missing source file. The following error message should instead include
"No such file or directory":
Temporary breakpoint 1, 0x00005555556f4de0 in main ()
(gdb) list
Downloading source file /usr/src/debug/glibc-2.36-8.fc37.x86_64/elf/<built-in>
1 /usr/src/debug/glibc-2.36-8.fc37.x86_64/elf/<built-in>: Directory not empty.
Fix this by having open_source_file return a negative errno if it fails
to open a source file. Use this value to generate the error message
instead of errno. Also add new function throw_sys_errmsg for reporting
this value.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29999
---
gdb/source-cache.c | 2 +-
gdb/source.c | 40 ++++++++++++++++++++++++++++++----------
gdb/source.h | 5 +++--
gdb/utils.c | 15 +++++++++++++++
gdb/utils.h | 2 ++
5 files changed, 51 insertions(+), 13 deletions(-)
Comments
On 1/27/23 19:37, Aaron Merey via Gdb-patches wrote:
>
> Fix this by having open_source_file return a negative errno if it fails
> to open a source file. Use this value to generate the error message
> instead of errno. Also add new function throw_sys_errmsg for reporting
> this value.
Thanks for the patch. This LGTM [warning: I am not a maintainer], but I just
have two observations (nits?).
> diff --git a/gdb/source.c b/gdb/source.c
> index be5af180c49..158b59fd613 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -1200,10 +1204,21 @@ open_source_file (struct symtab *s)
>
> /* Query debuginfod for the source file. */
> if (build_id != nullptr && !srcpath.empty ())
> - fd = debuginfod_source_query (build_id->data,
> - build_id->size,
> - srcpath.c_str (),
> - &fullname);
> + {
> + scoped_fd query_fd
> + = debuginfod_source_query (build_id->data,
> + build_id->size,
> + srcpath.c_str (),
> + &fullname);
> +
> + /* Don't return a negative errno from debuginfod_source_query.
> + It handles the reporting of its own errors. */
I would prefer to see a comment on this behavior with debuginfod_source_query's
description. [Repeating here is fine IMO.]
> + if (query_fd.get () >= 0)
> + {
> + s->fullname = fullname.release ();
> + return query_fd;
> + }
> + }
> }
> }
>
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 95adbe58e4a..0a5c6a70531 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -670,6 +670,21 @@ print_sys_errmsg (const char *string, int errcode)
> gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
> }
>
> +/* Same as perror_with_name except that ERRCODE specifies the error
> + instead of errno. */
> +
Should this comment be moved to the decl in utils.h? In such
cases, we normally use "See utils.h." as a comment at the
definition.
> +void
> +throw_sys_errmsg (const char *string, int errcode)
> +{
> + const char *err = safe_strerror (errcode);
> + std::string combined = std::string (string) + ": " + err;
> +
> + bfd_set_error (bfd_error_no_error);
> + errno = 0;
> +
> + throw_error (GENERIC_ERROR, _("%s."), combined.c_str ());
> +}
> +
> /* Control C eventually causes this to be called, at a convenient time. */
>
> void
> diff --git a/gdb/utils.h b/gdb/utils.h
> index 7865812998e..54a1bc4e476 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -278,6 +278,8 @@ extern void fprintf_symbol (struct ui_file *, const char *,
> extern void perror_warning_with_name (const char *string);
>
> extern void print_sys_errmsg (const char *, int);
> +
> +extern void throw_sys_errmsg (const char *, int) ATTRIBUTE_NORETURN;
>
> /* Warnings and error messages. */
<paranoid>FWIW, I've regression tested this on x86_64 both unix/-m64 and
native-extended-gdbserver/-m64. There were no issues.</paranoid>
Keith
On 01/02/2023 20:45, Keith Seitz via Gdb-patches wrote:
> On 1/27/23 19:37, Aaron Merey via Gdb-patches wrote:
>>
>> Fix this by having open_source_file return a negative errno if it fails
>> to open a source file. Use this value to generate the error message
>> instead of errno. Also add new function throw_sys_errmsg for reporting
>> this value.
>
> Thanks for the patch. This LGTM [warning: I am not a maintainer], but
> I just
> have two observations (nits?).
Friendly reminder that this is exactly what reviewed-by and approved-by
tags were introduced to solve :-)
>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
Aaron> Fix this by having open_source_file return a negative errno if it fails
Aaron> to open a source file. Use this value to generate the error message
Aaron> instead of errno. Also add new function throw_sys_errmsg for reporting
Aaron> this value.
I wonder if it's guaranteed somewhere that the errno values are always
positive.
Aaron> +/* Same as perror_with_name except that ERRCODE specifies the error
Aaron> + instead of errno. */
Aaron> +
Aaron> +void
Aaron> +throw_sys_errmsg (const char *string, int errcode)
IMO probably better to just add a parameter to perror_with_name that
defaults to 0, meaning use errno.
However, if you want to go this route, a function that always throws
must be marked ATTRIBUTE_NORETURN.
Tom
@@ -95,7 +95,7 @@ source_cache::get_plain_source_lines (struct symtab *s,
{
scoped_fd desc (open_source_file (s));
if (desc.get () < 0)
- perror_with_name (symtab_to_filename_for_display (s));
+ throw_sys_errmsg (symtab_to_filename_for_display (s), -desc.get ());
struct stat st;
if (fstat (desc.get (), &st) < 0)
@@ -1069,7 +1069,7 @@ find_and_open_source (const char *filename,
the attempt to read this source file failed. GDB will then display
the filename and line number instead. */
if (!source_open)
- return scoped_fd (-1);
+ return scoped_fd (-ECANCELED);
/* Quick way out if we already know its full name. */
if (*fullname)
@@ -1160,11 +1160,15 @@ find_and_open_source (const char *filename,
OPEN_MODE, fullname);
}
+ /* If the file wasn't found, then openp will have set errno accordingly. */
+ if (result < 0)
+ result = -errno;
+
return scoped_fd (result);
}
/* Open a source file given a symtab S. Returns a file descriptor or
- negative number for error.
+ negative errno for error.
This function is a convenience function to find_and_open_source. */
@@ -1172,7 +1176,7 @@ scoped_fd
open_source_file (struct symtab *s)
{
if (!s)
- return scoped_fd (-1);
+ return scoped_fd (-EINVAL);
gdb::unique_xmalloc_ptr<char> fullname (s->fullname);
s->fullname = NULL;
@@ -1200,10 +1204,21 @@ open_source_file (struct symtab *s)
/* Query debuginfod for the source file. */
if (build_id != nullptr && !srcpath.empty ())
- fd = debuginfod_source_query (build_id->data,
- build_id->size,
- srcpath.c_str (),
- &fullname);
+ {
+ scoped_fd query_fd
+ = debuginfod_source_query (build_id->data,
+ build_id->size,
+ srcpath.c_str (),
+ &fullname);
+
+ /* Don't return a negative errno from debuginfod_source_query.
+ It handles the reporting of its own errors. */
+ if (query_fd.get () >= 0)
+ {
+ s->fullname = fullname.release ();
+ return query_fd;
+ }
+ }
}
}
@@ -1306,6 +1321,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
print_source_lines_flags flags)
{
bool noprint = false;
+ int errcode = ENOENT;
int nlines = stopline - line;
struct ui_out *uiout = current_uiout;
@@ -1336,7 +1352,10 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
scoped_fd desc = open_source_file (s);
last_source_error = desc.get () < 0;
if (last_source_error)
- noprint = true;
+ {
+ noprint = true;
+ errcode = -desc.get ();
+ }
}
}
else
@@ -1354,7 +1373,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
char *name = (char *) alloca (len);
xsnprintf (name, len, "%d\t%s", line, filename);
- print_sys_errmsg (name, errno);
+ print_sys_errmsg (name, errcode);
}
else
{
@@ -1627,7 +1646,8 @@ search_command_helper (const char *regex, int from_tty, bool forward)
scoped_fd desc (open_source_file (loc->symtab ()));
if (desc.get () < 0)
- perror_with_name (symtab_to_filename_for_display (loc->symtab ()));
+ throw_sys_errmsg (symtab_to_filename_for_display (loc->symtab ()),
+ -desc.get ());
int line = (forward
? last_line_listed + 1
@@ -67,7 +67,8 @@ extern void init_source_path (void);
The caller is responsible for freeing FULLNAME.
On Failure
- An invalid file descriptor is returned (the return value is negative).
+ An invalid file descriptor is returned. The value of this file
+ descriptor is a negative errno indicating the reason for the failure.
FULLNAME is set to NULL. */
extern scoped_fd find_and_open_source (const char *filename,
const char *dirname,
@@ -81,7 +82,7 @@ extern gdb::unique_xmalloc_ptr<char> find_source_or_rewrite
(const char *filename, const char *dirname);
/* Open a source file given a symtab S. Returns a file descriptor or
- negative number for error. */
+ negative errno indicating the reason for the failure. */
extern scoped_fd open_source_file (struct symtab *s);
extern gdb::unique_xmalloc_ptr<char> rewrite_source_path (const char *path);
@@ -670,6 +670,21 @@ print_sys_errmsg (const char *string, int errcode)
gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
}
+/* Same as perror_with_name except that ERRCODE specifies the error
+ instead of errno. */
+
+void
+throw_sys_errmsg (const char *string, int errcode)
+{
+ const char *err = safe_strerror (errcode);
+ std::string combined = std::string (string) + ": " + err;
+
+ bfd_set_error (bfd_error_no_error);
+ errno = 0;
+
+ throw_error (GENERIC_ERROR, _("%s."), combined.c_str ());
+}
+
/* Control C eventually causes this to be called, at a convenient time. */
void
@@ -278,6 +278,8 @@ extern void fprintf_symbol (struct ui_file *, const char *,
extern void perror_warning_with_name (const char *string);
extern void print_sys_errmsg (const char *, int);
+
+extern void throw_sys_errmsg (const char *, int) ATTRIBUTE_NORETURN;
/* Warnings and error messages. */