Re: [PATCH] libelf: Only use posix_fallocate when using mmap. Ignore unsupported errno.

Re: [PATCH] libelf: Only use posix_fallocate when using mmap. Ignore unsupported errno.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 237 bytes --]

On Wed, 2015-10-07 at 16:57 +0200, Mark Wielaard wrote:
> So I changed the code again, now we just ignore all errors, except for
> ENOSPC since that is the only real thing we are interested in.

I pushed this variant to master now.

Re: [PATCH] libelf: Only use posix_fallocate when using mmap. Ignore unsupported errno.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]

On Mon, 2015-10-05 at 17:39 +0200, Mark Wielaard wrote:
> When not using mmap it is enough the just ftruncate the file to the right
> size. pwrite will report an error if there is no disk space left. And on
> file systems that don't support fallocate it might duplicate writes. When
> using posix_fallocate do ignore errors indicating the file system doesn't
> support fallocate. These will never happen with glibc, but might on some
> other implementations. That is pretty nasty since we might get a SIGBUS
> in that case when writing to the mmapped memory. But the chance of that
> happening is very small and people using such a libc implementation are
> on their own.

Sigh. Another issue with posix_fallocate was pointed out, this time in
the glibc fallback code, which introduces yet another failure mode/errno
to consider... I am starting to be a little less enthusiastic about this
function... If the original fd underlying the ELF file is opened
O_WRONLY then posix_fallocate might fail with EBADF if the glibc
fallback code is triggered. This doesn't actually happen right now in
our code, since if elf_begin is called with ELF_C_WRITE_MMAP (and not
ELF_C_RDWR_MMAP) we don't actually mmap the underlying file (yes, I was
surprised too, but ELF_C_WRITE and ELF_C_WRITE_MMAP both go through
write_file, which doesn't do mmap). But in case we ever change that for
some reason then having to handle yet another errno/failure case in some
obscure situations will be a pain.

So I changed the code again, now we just ignore all errors, except for
ENOSPC since that is the only real thing we are interested in.

Cheers,

Mark

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libelf-Only-use-posix_fallocate-when-using-mmap.-Ign.patch --]
[-- Type: text/x-patch, Size: 4496 bytes --]

From e589a11321ec1906de0e94ae7f9df5d180e873d9 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Mon, 5 Oct 2015 17:32:29 +0200
Subject: [PATCH] libelf: Only use posix_fallocate when using mmap. Ignore
 unsupported errors.

Don't use posix_fallocate when not using mmap. It is enough to ftruncate
the file to the right size. pwrite will report an error if there is no
disk space left. And on file systems that don't support fallocate it
might duplicate writes in that case. When using posix_fallocate do ignore
most errors. Other libc implementations don't guarantee the call actually
works always and even with glibc there might be an unexpected error from
the fallback code when the file system doesn't support fallocate. That is
pretty nasty since we might get a SIGBUS in that case when writing to the
mmapped memory. But the chance of that happening is very small.  And will
normally never happen with glibc. So only report an error when
posix_fallocate reports ENOSPC.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libelf/ChangeLog    |  5 +++++
 libelf/elf_update.c | 36 +++++++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 1faa9c2..56adc9f 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2015-10-05  Mark Wielaard  <mjw@redhat.com>
+
+	* elf_update.c (write_file): Only use posix_fallocate when using
+	mmap. Only report failure when errno is ENOSPC.
+
 2015-10-05  Josh Stone  <jistone@redhat.com>
 
 	* Makefile.am (libelf.so): Add AM_V_CCLD and AM_V_at silencers.
diff --git a/libelf/elf_update.c b/libelf/elf_update.c
index 00f7a01..c635eb3 100644
--- a/libelf/elf_update.c
+++ b/libelf/elf_update.c
@@ -57,22 +57,11 @@ write_file (Elf *elf, off_t size, int change_bo, size_t shnum)
      We cannot do this if this file is in an archive.  We also don't
      do it *now* if we are shortening the file since this would
      prevent programs to use the data of the file in generating the
-     new file.  We truncate the file later in this case.
-
-     Note we use posix_fallocate to make sure the file content is really
-     there. Only using ftruncate might mean the file is extended, but space
-     isn't allocated yet. This might cause a SIGBUS once we write into
-     the mmapped space and the disk is full. Using fallocate might fail
-     on some file systems. posix_fallocate is required to extend the file
-     and allocate enough space even if the underlying filesystem would
-     normally return EOPNOTSUPP.  Note that we do also need to ftruncate
-     in case the maximum_size isn't known and the file needs to be shorter
-     because posix_fallocate can only extend.  */
+     new file.  We truncate the file later in this case.  */
   if (elf->parent == NULL
       && (elf->maximum_size == ~((size_t) 0)
 	  || (size_t) size > elf->maximum_size)
-      && unlikely (ftruncate (elf->fildes, size) != 0)
-      && unlikely (posix_fallocate (elf->fildes, 0, size) != 0))
+      && unlikely (ftruncate (elf->fildes, size) != 0))
     {
       __libelf_seterrno (ELF_E_WRITE_ERROR);
       return -1;
@@ -89,6 +78,27 @@ write_file (Elf *elf, off_t size, int change_bo, size_t shnum)
 
   if (elf->map_address != NULL)
     {
+      /* When using mmap we want to make sure the file content is
+	 really there. Only using ftruncate might mean the file is
+	 extended, but space isn't allocated yet.  This might cause a
+	 SIGBUS once we write into the mmapped space and the disk is
+	 full.  In glibc posix_fallocate is required to extend the
+	 file and allocate enough space even if the underlying
+	 filesystem would normally return EOPNOTSUPP.  But other
+	 implementations might not work as expected.  And the glibc
+	 fallback case might fail (with unexpected errnos) in some cases.
+	 So we only report an error when the call fails and errno is
+	 ENOSPC. Otherwise we ignore the error and treat it as just hint.  */
+      if (elf->parent == NULL
+	  && (elf->maximum_size == ~((size_t) 0)
+	      || (size_t) size > elf->maximum_size)
+	  && unlikely (posix_fallocate (elf->fildes, 0, size) != 0))
+	if (errno == ENOSPC)
+	  {
+	    __libelf_seterrno (ELF_E_WRITE_ERROR);
+	    return -1;
+	  }
+
       /* The file is mmaped.  */
       if ((class == ELFCLASS32
 	   ? __elf32_updatemmap (elf, change_bo, shnum)
-- 
1.8.3.1

[PATCH] libelf: Only use posix_fallocate when using mmap. Ignore unsupported errno.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 3987 bytes --]

When not using mmap it is enough the just ftruncate the file to the right
size. pwrite will report an error if there is no disk space left. And on
file systems that don't support fallocate it might duplicate writes. When
using posix_fallocate do ignore errors indicating the file system doesn't
support fallocate. These will never happen with glibc, but might on some
other implementations. That is pretty nasty since we might get a SIGBUS
in that case when writing to the mmapped memory. But the chance of that
happening is very small and people using such a libc implementation are
on their own.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libelf/ChangeLog    |  5 +++++
 libelf/elf_update.c | 35 ++++++++++++++++++++++-------------
 2 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 1916877..8a08c5f 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2015-10-05  Mark Wielaard  <mjw@redhat.com>
+
+	* elf_update.c (write_file): Only use posix_fallocate when using
+	mmap. Don't fail on EINVAL or EOPNOTSUPP.
+
 2015-09-23  Mark Wielaard  <mjw@redhat.com>
 
 	* elf32_getehdr.c (getehdr_wrlock): Mark as internal_function.
diff --git a/libelf/elf_update.c b/libelf/elf_update.c
index 00f7a01..3d27f7f 100644
--- a/libelf/elf_update.c
+++ b/libelf/elf_update.c
@@ -57,22 +57,11 @@ write_file (Elf *elf, off_t size, int change_bo, size_t shnum)
      We cannot do this if this file is in an archive.  We also don't
      do it *now* if we are shortening the file since this would
      prevent programs to use the data of the file in generating the
-     new file.  We truncate the file later in this case.
-
-     Note we use posix_fallocate to make sure the file content is really
-     there. Only using ftruncate might mean the file is extended, but space
-     isn't allocated yet. This might cause a SIGBUS once we write into
-     the mmapped space and the disk is full. Using fallocate might fail
-     on some file systems. posix_fallocate is required to extend the file
-     and allocate enough space even if the underlying filesystem would
-     normally return EOPNOTSUPP.  Note that we do also need to ftruncate
-     in case the maximum_size isn't known and the file needs to be shorter
-     because posix_fallocate can only extend.  */
+     new file.  We truncate the file later in this case.  */
   if (elf->parent == NULL
       && (elf->maximum_size == ~((size_t) 0)
 	  || (size_t) size > elf->maximum_size)
-      && unlikely (ftruncate (elf->fildes, size) != 0)
-      && unlikely (posix_fallocate (elf->fildes, 0, size) != 0))
+      && unlikely (ftruncate (elf->fildes, size) != 0))
     {
       __libelf_seterrno (ELF_E_WRITE_ERROR);
       return -1;
@@ -89,6 +78,26 @@ write_file (Elf *elf, off_t size, int change_bo, size_t shnum)
 
   if (elf->map_address != NULL)
     {
+      /* When using mmap we want to make sure the file content is
+	 really there. Only using ftruncate might mean the file is
+	 extended, but space isn't allocated yet.  This might cause a
+	 SIGBUS once we write into the mmapped space and the disk is
+	 full.  In glibc posix_fallocate is required to extend the
+	 file and allocate enough space even if the underlying
+	 filesystem would normally return EOPNOTSUPP.  But other
+	 implementations might not work as expected.  So we ignore
+	 EOPNOTSUPP and EINVAL, which posix specifies as alternative,
+	 errors here.  */
+      if (elf->parent == NULL
+	  && (elf->maximum_size == ~((size_t) 0)
+	      || (size_t) size > elf->maximum_size)
+	  && unlikely (posix_fallocate (elf->fildes, 0, size) != 0))
+	if (errno != EINVAL && errno != EOPNOTSUPP)
+	  {
+	    __libelf_seterrno (ELF_E_WRITE_ERROR);
+	    return -1;
+	  }
+
       /* The file is mmaped.  */
       if ((class == ELFCLASS32
 	   ? __elf32_updatemmap (elf, change_bo, shnum)
-- 
1.8.3.1