Bug 54962 - Apache will set incorrect Content-Length header and may serve partial files
Summary: Apache will set incorrect Content-Length header and may serve partial files
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: Core (show other bugs)
Version: 2.5-HEAD
Hardware: All All
: P2 major (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: PatchAvailable
: 47693 64834 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-05-14 05:57 UTC by Richard Fliam
Modified: 2020-10-22 10:07 UTC (History)
3 users (show)



Attachments
small program to provoke the error (839 bytes, text/x-csrc)
2020-03-27 17:12 UTC, Ståle Kristoffersen
Details
This patch solves the problem for me by getting the size from the open file descriptor. (420 bytes, patch)
2020-03-30 14:11 UTC, Ståle Kristoffersen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Fliam 2013-05-14 05:57:51 UTC
We have several files which are moved (renamed) on top of frequently (about every 2 seconds). 

A rename or move operation is atomic in the kernel, and thus should not cause issues. However, due to the nature of the way Apache serves these files it will periodically serve an incorrect Content-Length header and a truncated file or not enough bits.

Steps to Reproduce:
Take a file, perhaps randomly generated.
Move (mv, or c rename()) a new one of differing size onto it with a relatively high frequency.

Cause:
Apache "stats" a file on receiving a request. It than opens a file descriptor for that file on line 4314 of core.c, and sets the content length header using `ap_set_content_length(r, r->finfo.size)` on 4328. This is the core of the problem, r->finfo.size is from a `stat()` that may not be the same inode.

The better (correct?) behavior is to use `fstat()` on the open file descriptor to set the Content-Length header as well as send size for sendfile etc. As file descriptors refer to an inode in most operating systems this prevents the underlying race on files which are being (atomically) updated.

To be clear this is not the same as having issues reading from a file that is being modified, the file is updated atomically and many other application (such as Python) handle this correctly.
Comment 1 Ståle Kristoffersen 2020-03-27 17:12:37 UTC
Created attachment 37131 [details]
small program to provoke the error

This program, compiled with gcc -o loop test.c will generate random length files.
The file "/usr/local/apache2/htdocs/test.txt" will be swapped out atomically, and will always be consistent on disk with the end of the file be the line "End of file!\n".

When fetching this through the latest Apache (compiled from httpd-2.4.41.tar.gz, apr-1.7.0.tar.gz and apr-util-1.6.1.tar.gz) it sometimes works as expected, sometimes I get a partial file (but the last line is then always "Lorem ipsum d", which is the same length as "End of line!\n").
Comment 2 Ståle Kristoffersen 2020-03-30 14:11:35 UTC
Created attachment 37135 [details]
This patch solves the problem for me by getting the size from the open file descriptor.
Comment 3 Joe Orton 2020-10-22 08:37:15 UTC
*** Bug 64834 has been marked as a duplicate of this bug. ***
Comment 4 Joe Orton 2020-10-22 08:37:27 UTC
*** Bug 47693 has been marked as a duplicate of this bug. ***
Comment 5 Joe Orton 2020-10-22 08:50:47 UTC
The problem here is that we're calling fstat() again for every request in the critical path of serving a GET request.  I don't know how much performance difference this will make on a modern server, but assume it is non-zero.

If we're going to do this it should be right after the open() so the mtime used is using correct stat data as well (and etag).

The trade-off is whether we should fix correctness for an edge case, but make everybody take a performance hit.

I think it should be possible to offer a "fast" equivalent of the default handler, i.e. without the extra fstat(), with some option/config tweak applied, BUT:

1. we can only do so by omitting Last-Modified, etag, and content-length, since all may be using stale stat data from the directory walk.  

2. we could not use the FILE bucket, which needs a determinate length (hence requires stat data), so we would lose the performance benefits sending the content via mmap() or sendfile() etc so it might not actually be faster (I have no idea)

3. for small files without a C-L, the content-length filter will still be able to generate a content-length even by reading till EOF, but not for larger files, so there is a performance/network efficiency hit here too.

I tend towards saying we should fix correctness at the cost of performance regardless, and probably the second-fstat-less path isn't worth the complexity/costs.

(The "proper" solution is to open the file then fstat in the dirwalk, then use the fd in the default handler, I have no idea how we can do it without huge hacks or some redesign of the core hooks, since we don't know which handler will be used in the dirwalk.  I may be missing other fundamental things.)
Comment 6 Ruediger Pluem 2020-10-22 10:07:32 UTC
(In reply to Joe Orton from comment #5)
> The problem here is that we're calling fstat() again for every request in
> the critical path of serving a GET request.  I don't know how much
> performance difference this will make on a modern server, but assume it is
> non-zero.
> 
> If we're going to do this it should be right after the open() so the mtime
> used is using correct stat data as well (and etag).
> 
> The trade-off is whether we should fix correctness for an edge case, but
> make everybody take a performance hit.
> 
> I think it should be possible to offer a "fast" equivalent of the default
> handler, i.e. without the extra fstat(), with some option/config tweak
> applied, BUT:
> 
> 1. we can only do so by omitting Last-Modified, etag, and content-length,
> since all may be using stale stat data from the directory walk.  
> 
> 2. we could not use the FILE bucket, which needs a determinate length (hence
> requires stat data), so we would lose the performance benefits sending the
> content via mmap() or sendfile() etc so it might not actually be faster (I
> have no idea)
> 
> 3. for small files without a C-L, the content-length filter will still be
> able to generate a content-length even by reading till EOF, but not for
> larger files, so there is a performance/network efficiency hit here too.
> 
> I tend towards saying we should fix correctness at the cost of performance
> regardless, and probably the second-fstat-less path isn't worth the
> complexity/costs.

+1

> 
> (The "proper" solution is to open the file then fstat in the dirwalk, then
> use the fd in the default handler, I have no idea how we can do it without
> huge hacks or some redesign of the core hooks, since we don't know which
> handler will be used in the dirwalk.  I may be missing other fundamental
> things.)

The idea to fix it that way instead of with the proposed patch to the default handler is to avoid similar issues for other handlers and having it to fix there over and over again? I agree this could be tricky to fix in the dirwalk. Maybe an additional fd field in request_rec? But this would mean we would always open the file regardless if the handler needs it or uses our fd.
I guess this is better discussed on dev@ than here.