[Bug libc/15661] New: posix_fallocate fallback code buggy and dangerous

[Bug libc/15661] New: posix_fallocate fallback code buggy and dangerous

[bugdal at aerifal dot cx]

http://sourceware.org/bugzilla/show_bug.cgi?id=15661

            Bug ID: 15661
           Summary: posix_fallocate fallback code buggy and dangerous
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: bugdal at aerifal dot cx
                CC: drepper.fsp at gmail dot com

When the fallocate syscall returns EOPNOTSUPP, glibc tries to emulate it via
checking the file size and, if the file size needs to be extended, writing a
single zero byte at the location that should be the new last byte of the file,
via pwrite. This approach is broken for two reasons:

1. For files opened in O_APPEND mode, pwrite will simply write at the current
position, extending the file by 1 byte rather than the correct amount.

2. Checking the size then writing at a location past the end of the file
involves a race condition that could lead to data corruption. If the file is
extended by another thread or process after the size is checked but before
pwrite is called, pwrite may overwrite data rather than writing to a new
location past the end of the file.

Problem 1 could be fixed via a larger write of zeros, but I see no fix for
problem 2. As such, I believe the buggy fallback code in posix_fallocate should
simply be removed, and the function should return an error to the application
rather than trying to emulate it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[bugdal at aerifal dot cx]

http://sourceware.org/bugzilla/show_bug.cgi?id=15661

--- Comment #1 from Rich Felker <bugdal at aerifal dot cx> ---
If keeping the emulation is desirable, I do have one potential fix that would
avoid the race condition. fork a child process (because this might SIGBUS if
another thread or process truncates the file), possibly using clone to avoid
the parent receiving signals from the child's termination. In the child, mmap
the file and use atomic compare-and-swap on each page that should be allocated,
with the comparison and new values being equal. This will touch each page with
a write in a way that's non-destructive.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[zlynx at acm dot org]

http://sourceware.org/bugzilla/show_bug.cgi?id=15661

Jonathan Briggs <zlynx at acm dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zlynx at acm dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com
              Flags|                            |security-

--- Comment #2 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Rich Felker from comment #1)
> If keeping the emulation is desirable, I do have one potential fix that
> would avoid the race condition. fork a child process (because this might
> SIGBUS if another thread or process truncates the file), possibly using
> clone to avoid the parent receiving signals from the child's termination. In
> the child, mmap the file and use atomic compare-and-swap on each page that
> should be allocated, with the comparison and new values being equal. This
> will touch each page with a write in a way that's non-destructive.

This needs a way to transparently create child processes, without delivering
SIGCHLD.  I don't think we have that at present.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

--- Comment #3 from Rich Felker <bugdal at aerifal dot cx> ---
In comment #1, I suggested using clone with a signal/flags argument of 0 to
avoid generating SIGCHLD. To wait for such a child, you use waidpid with
__WCLONE. So I think there is a viable workaround, but I'm not sure if it's
desirable. Just returning an error rather than attempting to emulate the
operation may be preferable. In any case, the current behavior should be
removed ASAP because it causes file corruption.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[Jean-Baptiste.Leonesio@esi-group.com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

Jean-Baptiste Leonesio <Jean-Baptiste.Leonesio@esi-group.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |Jean-Baptiste.Leonesio@esi-
                   |                            |group.com

--- Comment #4 from Jean-Baptiste Leonesio <Jean-Baptiste.Leonesio@esi-group.com> ---
Created attachment 8266
  --> https://sourceware.org/bugzilla/attachment.cgi?id=8266&action=edit
posix_fallocate minimal testcast

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[Jean-Baptiste.Leonesio@esi-group.com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

--- Comment #5 from Jean-Baptiste Leonesio <Jean-Baptiste.Leonesio@esi-group.com> ---
We stepped upon this exact same bug when using a ramfs in order to remove IO
bottlenecks when doing some benchmarks.
Although working with a tmpfs is simple workaround, it still took us some time
to figure out what was going wrong with our integrity test suite.

Moreover, we second the fact that data corruption should not happen since it
can lead to way more serious trouble than simply losing a couple of days.

If properly fixing the race condition is not possible, POSIX.1-2001 defines the
error EINVAL as "[...] or the underlying file system does not support this
operation" but it would break the sequential behavior which is working.

Enclosed a minimal testcase reproducing the current file corruption behavior
when writing data on a ramfs in a multithreaded fashion.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

--- Comment #6 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Rich Felker from comment #1)
> If keeping the emulation is desirable, I do have one potential fix that
> would avoid the race condition. fork a child process (because this might
> SIGBUS if another thread or process truncates the file), possibly using
> clone to avoid the parent receiving signals from the child's termination. In
> the child, mmap the file and use atomic compare-and-swap on each page that
> should be allocated, with the comparison and new values being equal. This
> will touch each page with a write in a way that's non-destructive.

This assumes a unified page cache, which is probably not even true for all
Linux file systems.  I think it can still result in data loss.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at sourceware dot org   |fweimer at redhat dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlos at redhat dot com

--- Comment #7 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Rich Felker from comment #0)
> When the fallocate syscall returns EOPNOTSUPP, glibc tries to emulate it via
> checking the file size and, if the file size needs to be extended, writing a
> single zero byte at the location that should be the new last byte of the
> file, via pwrite. This approach is broken for two reasons:
> 
> 1. For files opened in O_APPEND mode, pwrite will simply write at the
> current position, extending the file by 1 byte rather than the correct
> amount.

This is a real bug.

> 2. Checking the size then writing at a location past the end of the file
> involves a race condition that could lead to data corruption. If the file is
> extended by another thread or process after the size is checked but before
> pwrite is called, pwrite may overwrite data rather than writing to a new
> location past the end of the file.

Given that posix_fallocate is not listed in POSIX section 2.9.7 "Thread
Interactions with Regular File Operations" the call is not required to be
atomic with respect to any other operation. Therefore it is the responsibility
of the "other thread" to synchronize the file operations with the thread
calling posix_fallocate to ensure posix_fallocate has returned.

Does that justify keeping the fallback code?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

--- Comment #8 from Rich Felker <bugdal at aerifal dot cx> ---
I think the XSH 2.9.7 justification is something of a cop-out, and the behavior
is still undesirable/QoI-issue, but at least it's arguably conforming to the
extent that applications should avoid depending on stricter behavior.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

--- Comment #9 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Rich Felker from comment #8)
> I think the XSH 2.9.7 justification is something of a cop-out, and the
> behavior is still undesirable/QoI-issue, but at least it's arguably
> conforming to the extent that applications should avoid depending on
> stricter behavior.

I agree with you, but I'm torn. There are definately applications that rely on
the fallback to work, and if we remove the fallback we'll have a ton of
applications doing the allocation loop themselves, likely getting the overflow
checking wrong, and the blocksize computation wrong. Therefore in the interest
of public safety it seems best to simply leave the loop in place.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #11 from Florian Weimer <fweimer at redhat dot com> ---
Commit 7fe9e2e089f4990b7d18d0798f591ab276b15f2b in 2.22 improves the situation
somewhat, but we cannot fix this interface for all users, so the bug has to be
remain WONTFIX (or INVALID if you wish, per comment #7).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://sourceware.org/bugz
                   |                            |illa/show_bug.cgi?id=17322

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[zlynx at acm dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

--- Comment #12 from Jonathan Briggs <zlynx at acm dot org> ---
(In reply to Rich Felker from comment #8)
> I think the XSH 2.9.7 justification is something of a cop-out, and the
> behavior is still undesirable/QoI-issue, but at least it's arguably
> conforming to the extent that applications should avoid depending on
> stricter behavior.

This is definitely a cop-out and very undesirable. Absolutely no one will
expect apparently random zero bytes to appear in their files because of calling
posix_fallocate because the function isn't supposed to write anything to the
file.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15661] posix_fallocate fallback code buggy and dangerous

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15661

--- Comment #13 from Carlos O'Donell <carlos at redhat dot com> ---
On 06/05/2015 12:52 PM, zlynx at acm dot org wrote:
> https://sourceware.org/bugzilla/show_bug.cgi?id=15661
> 
> --- Comment #12 from Jonathan Briggs <zlynx at acm dot org> ---
> (In reply to Rich Felker from comment #8)
>> I think the XSH 2.9.7 justification is something of a cop-out, and the
>> behavior is still undesirable/QoI-issue, but at least it's arguably
>> conforming to the extent that applications should avoid depending on
>> stricter behavior.
> 
> This is definitely a cop-out and very undesirable. Absolutely no one will
> expect apparently random zero bytes to appear in their files because of calling
> posix_fallocate because the function isn't supposed to write anything to the
> file.

I think there are pros and cons to both sides. The position of consensus
among the conservative glibc developers is to leave the fallback in place
as it has been for a long time, and adjust the documentation to educate
developers on the issue. Please help us spread the word about how the
function operates and that a portable use involves synchronization.

Someone should patch the man page to talk about the fallback when using
posix_fallocate on a filesystem that doesn't implement fallocate.

Cheers,
Carlos.

-- 
You are receiving this mail because:
You are on the CC list for the bug.