alignment check breaks trailer code for ngx_freebsd_sendfile_chain.c, possibly others

[mattdillon.pip.verisignlabs.com]

The ngx_freebsd_sendfile_chain.c file contains sendfile() code for FreeBSD (and also DragonFly). This code implements sendfile() with headers, file body, and trailers. This bug pertains to an alignment reduction done in the file body which can cause the trailers to operate improperly.

Around line 160 in os/unix/ngx_freebsd_sendfile_chain.c the following code is present:

if (send + size > limit) {

size = limit - send;

aligned = (cl->buf->file_pos + size + ngx_pagesize - 1)

& ~((off_t) ngx_pagesize - 1);

if (aligned <= cl->buf->file_last) {

size = aligned - cl->buf->file_pos;

}

}

The problem is that if this alignment code is *EVER* hit and the (size) variable is reduced, the (cl) variable will be advanced to the next chain but (send + size) will remain < (limit). This will allow the trailer-generating code which follows, around line 183, to execute at least one loop on the chain following the file body chain and possibly improperly add a trailer to the sendfile() sequence when the file body has not yet been completed.

I do not believe that this bug gets hit for normal file sends, but any partial resend can potentially hit this bug and cause corruption in the data stream.

The solution to this problem is to test for the case and forcefully skip the the trailer code if it gets hit. So, e.g.

if (aligned <= cl->buf->file_last) {

size = aligned - cl->buf->file_pos;
goto skip_trailers;

}

... (and then add a skip_trailers: label down after the trailers block)

I came across this bug while examining the code debugging a DragonFly sendfile() issue. That issue was a kernel bug and not this bug, but insofar as I can tell this is a bug too. I have not tested that the case can actually occur but it seems like it should be possible for it to occur in client/server systems that request files piecemeal in an unaligned fashion.

I believe that this bug can only occur in situations where trailers are used with sendfile(). FreeBSD and DragonFly are certain affected. In situations where trailers are not implemented, the code bug is present but the advanced chain is thrown away so it isn't a problem.

-Matt

[mattdillon.pip.verisignlabs.com]

My sample solution code is wrong. The goto cannot be done there because file_size, send, and fprev are not updated at that point.

Instead of placing the goto one can simply prevent the cl variable from being advanced and break out of the file body loop as follows:

file_size += (size_t) size;
send += size;
fprev = cl->buf->file_pos + size;
if (fprev != cl->buf->file_last) /* ADDED CODE */

break; /* ADDED CODE */

cl = cl->next;

[maxim]

Thanks, Matt. We will take a look.

[Ruslan Ermilov]

Matt,

I think you're wrong on your assumption that the "size" variable is reduced in the alignment case.

"size" is first initialized to the size of the file buffer. If it would exceed the "limit", "size" is truncated so that (send + size == limit). Then,

size = aligned - cl->buf->file_pos;

could make "size" only bigger, by rounding up the right end of the range to the page size, thus the condition (send + size >= limit) holds true.

"send" is then incremented by "size", so "send < limit" will be false, and no trailer code will be run.