Re: [PATCH] libelf: Use possix_fallocate instead of ftruncate to extend ELF file.

Re: [PATCH] libelf: Use possix_fallocate instead of ftruncate to extend ELF file.

[Mark Wielaard]

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

On Fri, 2015-05-15 at 12:08 +0200, Mark Wielaard wrote:
> Fixed it everywhere and updated the mjw/pending branch. Not pushing yet.
> I would like to know what others think about using posix_fallocate
> instead of ftruncate. There is some discussion on the glibc mailinglist
> about whether or not posix_fallocate actually guarantees to extend the
> file or not. I think it does, that is the main difference with fallocate
> (and ftruncate). But lets see what glibc ends up deciding.
> https://sourceware.org/ml/libc-alpha/2015-04/msg00309.html

Pushed now to master. It does look like using posix_fallocate in this
particular case over ftruncate is always better. Even if it
theoretically doesn't prevent the SIGBUS issue in all corner cases. In
practice it should.

Cheers,

Mark

Re: [PATCH] libelf: Use possix_fallocate instead of ftruncate to extend ELF file.

[Mark Wielaard]

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

On Thu, 2015-05-14 at 09:44 -0700, Josh Stone wrote:
> On 05/14/2015 03:44 AM, Mark Wielaard wrote:
> > diff --git a/libelf/ChangeLog b/libelf/ChangeLog
> > index 2d10b83..5e7b104 100644
> > --- a/libelf/ChangeLog
> > +++ b/libelf/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2015-05-14  Mark Wielaard  <mjw@redhat.com>
> > +
> > +	* elf_update.c (write_file): Use possix_fallocate instead of
> > +	ftruncate to extend file if necessary.
> 
> Just one 's' in posix -- also mistyped in the subject line.

Worse, also mistyped in the patch as posted though not in the commit on
the branch, since obviously I noticed it didn't compile and fixed it,
but somehow still posted the broken patch...

Fixed it everywhere and updated the mjw/pending branch. Not pushing yet.
I would like to know what others think about using posix_fallocate
instead of ftruncate. There is some discussion on the glibc mailinglist
about whether or not posix_fallocate actually guarantees to extend the
file or not. I think it does, that is the main difference with fallocate
(and ftruncate). But lets see what glibc ends up deciding.
https://sourceware.org/ml/libc-alpha/2015-04/msg00309.html

Cheers,

Mark

Re: [PATCH] libelf: Use possix_fallocate instead of ftruncate to extend ELF file.

[Josh Stone]

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

On 05/14/2015 03:44 AM, Mark Wielaard wrote:
> diff --git a/libelf/ChangeLog b/libelf/ChangeLog
> index 2d10b83..5e7b104 100644
> --- a/libelf/ChangeLog
> +++ b/libelf/ChangeLog
> @@ -1,3 +1,8 @@
> +2015-05-14  Mark Wielaard  <mjw@redhat.com>
> +
> +	* elf_update.c (write_file): Use possix_fallocate instead of
> +	ftruncate to extend file if necessary.

Just one 's' in posix -- also mistyped in the subject line.

[PATCH] libelf: Use possix_fallocate instead of ftruncate to extend ELF file.

[Mark Wielaard]

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

This fixes an obscure SIGBUS error when using ELF_C_WRITE_MMAP on an ELF
file that needs extending when the underlying file system is (nearly) full.

Use posix_fallocate to make sure the file content is really there. 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 or the kernel
doesn't implement the fallocate syscall. Also posix_fallocate has been in
glibc since 2.1.94, while support for fallocate was only added in 2.10
and kernel 2.6.23.

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

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 2d10b83..5e7b104 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2015-05-14  Mark Wielaard  <mjw@redhat.com>
+
+	* elf_update.c (write_file): Use possix_fallocate instead of
+	ftruncate to extend file if necessary.
+
 2015-05-13  Mark Wielaard  <mjw@redhat.com>
 
 	* elf32_updatenull.c (default_ehdr): If e_phnum is zero then set
diff --git a/libelf/elf_update.c b/libelf/elf_update.c
index 54c20f5..3674e72 100644
--- a/libelf/elf_update.c
+++ b/libelf/elf_update.c
@@ -1,5 +1,5 @@
 /* Update data structures for changes and write them out.
-   Copyright (C) 1999, 2000, 2001, 2002, 2004, 2005, 2006 Red Hat, Inc.
+   Copyright (C) 1999, 2000, 2001, 2002, 2004, 2005, 2006, 2015 Red Hat, Inc.
    This file is part of elfutils.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 1999.
 
@@ -32,6 +32,7 @@
 #endif
 
 #include <libelf.h>
+#include <fcntl.h>
 #include <unistd.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
@@ -56,11 +57,19 @@ 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.  */
+     new file.  We truncate the file later in this case.
+
+     Note we use posix_fallocate to make sure the file content is really
+     there. 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.  */
   if (elf->parent == NULL
       && (elf->maximum_size == ~((size_t) 0)
 	  || (size_t) size > elf->maximum_size)
-      && unlikely (ftruncate (elf->fildes, size) != 0))
+      && unlikely (possix_fallocate (elf->fildes, 0, size) != 0))
     {
       __libelf_seterrno (ELF_E_WRITE_ERROR);
       return -1;
@@ -94,6 +103,7 @@ write_file (Elf *elf, off_t size, int change_bo, size_t shnum)
 	size = -1;
     }
 
+  /* Reduce the file size if necessary.  */
   if (size != -1
       && elf->parent == NULL
       && elf->maximum_size != ~((size_t) 0)
-- 
1.8.3.1