gdb: check result of gdb_fopen_cloexec in dump_binary_file

Message ID 20210424225023.73339-1-simon.marchi@polymtl.ca
State New
Headers show
Series
  • gdb: check result of gdb_fopen_cloexec in dump_binary_file
Related show

Commit Message

Mike Frysinger via Gdb-patches April 24, 2021, 10:50 p.m.
Bug 27773 shows that passing a filename in a non-existent directory to
the "dump binary" command leads to a gdb crash.  This is because the
gdb_fopen_cloexec in dump_binary_file fails (returns nullptr) and the
return value is not checked.  Fix that by erroring out if
gdb_fopen_cloexec fails.

gdb/ChangeLog:

	PR gdb/27773
	* cli/cli-dump.c (dump_binary_file): Check result of
	gdb_fopen_cloexec.

gdb/testsuite/ChangeLog:

	PR gdb/27773
	* gdb.base/dump.exp: Test dump to non-existent dir.

Change-Id: Iea89a3bf9e6b9dcc31142faa5ae17bc855759328
---
 gdb/cli/cli-dump.c              |  3 +++
 gdb/testsuite/gdb.base/dump.exp | 10 ++++++++++
 2 files changed, 13 insertions(+)

-- 
2.30.1

Comments

Tom Tromey April 26, 2021, 2:17 p.m. | #1
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:


Simon> 	PR gdb/27773
Simon> 	* cli/cli-dump.c (dump_binary_file): Check result of
Simon> 	gdb_fopen_cloexec.

Looks good.

I wonder whether gdb_fopen_cloexec should be declared
ATTRIBUTE_WARN_UNUSED_RESULT.

Tom
Mike Frysinger via Gdb-patches April 26, 2021, 2:37 p.m. | #2
On 2021-04-26 10:17 a.m., Tom Tromey wrote:>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 

> Simon> 	PR gdb/27773

> Simon> 	* cli/cli-dump.c (dump_binary_file): Check result of

> Simon> 	gdb_fopen_cloexec.

> 

> Looks good.

> 

> I wonder whether gdb_fopen_cloexec should be declared

> ATTRIBUTE_WARN_UNUSED_RESULT.


Would that help in this case?  The return value is used, so I presume it
wouldn't warn.

Although it wouldn't hurt to have it, a call to gdb_fopen_cloexec that
doesn't use the return value would be completely useless (unless there's
a use case I don't see of opening the file and closing it right away).

I also wonder if the attribute is smart enough for when the return value
is a "smart" object like gdb_file_up, and not a plain pointer.  I'll
have to try.

Simon
Tom Tromey April 26, 2021, 3:25 p.m. | #3
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:


Simon> On 2021-04-26 10:17 a.m., Tom Tromey wrote:>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 

Simon> PR gdb/27773
Simon> * cli/cli-dump.c (dump_binary_file): Check result of
Simon> gdb_fopen_cloexec.
>> 

>> Looks good.

>> 

>> I wonder whether gdb_fopen_cloexec should be declared

>> ATTRIBUTE_WARN_UNUSED_RESULT.


Simon> Would that help in this case?  The return value is used, so I presume it
Simon> wouldn't warn.

Haha, I guess not.

Tom

Patch

diff --git a/gdb/cli/cli-dump.c b/gdb/cli/cli-dump.c
index ee3ce03be035..1df26626f9d2 100644
--- a/gdb/cli/cli-dump.c
+++ b/gdb/cli/cli-dump.c
@@ -135,6 +135,9 @@  dump_binary_file (const char *filename, const char *mode,
   int status;
 
   gdb_file_up file = gdb_fopen_cloexec (filename, mode);
+  if (file == nullptr)
+    perror_with_name (filename);
+
   status = fwrite (buf, len, 1, file.get ());
   if (status != 1)
     perror_with_name (filename);
diff --git a/gdb/testsuite/gdb.base/dump.exp b/gdb/testsuite/gdb.base/dump.exp
index 2b792373c6cd..52c698333d20 100644
--- a/gdb/testsuite/gdb.base/dump.exp
+++ b/gdb/testsuite/gdb.base/dump.exp
@@ -24,6 +24,8 @@  set options  {debug}
 set is64bitonly "no"
 set endian "auto"
 
+set formats {binary ihex srec tekhex  verilog}
+
 if [istarget "alpha*-*-*"] then {
     # SREC etc cannot handle 64-bit addresses.  Force the test
     # program into the low 31 bits of the address space.
@@ -467,6 +469,14 @@  if ![string compare $is64bitonly "no"] then {
 }
 
 
+# Test writing a file of each format to a directory that does not exist.
+
+foreach_with_prefix format $formats {
+    gdb_test "dump $format memory /tmp/non/existent/directory/file $array_start $array_end" \
+	"/tmp/non/existent/directory/file: No such file or directory." \
+	"dump to non-existent directory"
+}
+
 # Now start a fresh gdb session, and reload the saved value files.
 
 gdb_exit