This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] gold: don't assume read/pread never return short counts


When I was debugging the problem with binary_unittest (on a mingw
host) that turned out to be missing O_BINARY, I first thought that it
was failing because it didn't handle short reads.  That turned out not
to be the problem I was having, but it's still a lack of robustness in
the code that deserves to be fixed.

Any read or pread call is allowed to read less than the full amount
you asked for.  At least on POSIX-like systems using regular disk
files, this occurs rarely to never in practice, but the specification
of these functions has always been that it might happen and that the
application is responsibile for restarting after a partial read.

Output_file::open_base_file already handles this correctly (using read).
binary_unittest's Sized_binary_test (using read) and File_read::read
(using pread) are the other two places I found doing reads.  I find it
a little troublesome that none of these is set up so it could reasonably
use a common code path for the same logic.  But it's not clear how best
to reorganize things so they would share more, or if that's what you want.

Ok for trunk?  (I'd be glad to put it on the 2.23 branch too, but since
it's not fixing any known-observable bug, I'm not sure whether it's
preferable to put it there or not.)


Thanks,
Roland

gold/
2012-12-07  Roland McGrath  <mcgrathr@google.com>

	* testsuite/binary_unittest.cc (read_all): New function.
	(Sized_binary_test): Use it instead of ::read.
	* fileread.cc (do_read): Don't assume pread always reads the whole
	amount in a single call.

--- a/gold/fileread.cc
+++ b/gold/fileread.cc
@@ -1,6 +1,7 @@
 // fileread.cc -- read files for gold

-// Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+// Copyright 2006, 2007, 2008, 2009, 2010, 2011, 2012
+// Free Software Foundation, Inc.
 // Written by Ian Lance Taylor <iant@google.com>.

 // This file is part of gold.
@@ -389,16 +390,29 @@ File_read::do_read(off_t start,
section_size_type size, void* p)
   else
     {
       this->reopen_descriptor();
-      bytes = ::pread(this->descriptor_, p, size, start);
-      if (static_cast<section_size_type>(bytes) == size)
-	return;

-      if (bytes < 0)
-	{
-	  gold_fatal(_("%s: pread failed: %s"),
-		     this->filename().c_str(), strerror(errno));
-	  return;
-	}
+      char *read_ptr = static_cast<char *>(p);
+      off_t read_pos = start;
+      size_t to_read = size;
+      do
+        {
+          bytes = ::pread(this->descriptor_, read_ptr, to_read, read_pos);
+          if (bytes < 0)
+            {
+              gold_fatal(_("%s: pread failed: %s"),
+                         this->filename().c_str(), strerror(errno));
+              return;
+            }
+
+          read_pos += bytes;
+          read_ptr += bytes;
+          to_read -= bytes;
+          if (to_read == 0)
+            return;
+        }
+      while (bytes > 0);
+
+      bytes = size - to_read;
     }

   gold_fatal(_("%s: file too short: read only %lld of %lld bytes at %lld"),
--- a/gold/testsuite/binary_unittest.cc
+++ b/gold/testsuite/binary_unittest.cc
@@ -1,6 +1,6 @@
 // binary_unittest.cc -- test Binary_to_elf

-// Copyright 2008 Free Software Foundation, Inc.
+// Copyright 2008, 2012 Free Software Foundation, Inc.
 // Written by Ian Lance Taylor <iant@google.com>.

 // This file is part of gold.
@@ -37,6 +37,29 @@
 #include "test.h"
 #include "testfile.h"

+namespace
+{
+
+ssize_t
+read_all (int fd, unsigned char* buf, ssize_t size)
+{
+  ssize_t total_read = 0;
+  while (size > 0)
+    {
+      ssize_t nread = ::read(fd, buf, size);
+      if (nread < 0)
+        return nread;
+      if (nread == 0)
+        break;
+      buf += nread;
+      size -= nread;
+      total_read += nread;
+    }
+  return total_read;
+}
+
+} // End anonymous namespace.
+
 namespace gold_testsuite
 {

@@ -56,7 +79,7 @@ Sized_binary_test()
   int o = ::open(gold::program_name, O_RDONLY);
   CHECK(o >= 0);
   unsigned char* filedata = new unsigned char[st.st_size];
-  CHECK(::read(o, filedata, st.st_size) == st.st_size);
+  CHECK(read_all(o, filedata, st.st_size) == static_cast<ssize_t>(st.st_size));
   CHECK(::close(o) == 0);

   Binary_to_elf binary(static_cast<elfcpp::EM>(0xffff), size, big_endian,


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]