httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Robinson <d...@esi.co.uk>
Subject Re: [PATCH] must flush for CRLFs after POSTs
Date Tue, 28 Jan 1997 11:21:56 GMT
On Tue, 28 Jan 1997, Dean Gaudet wrote:

> Here's a patch to deal with the problem Alexei brought up regarding
> clients sending an extra CRLF after a POST which isn't included in the
> Content-Length.  It tweaks buff.c so that it knows to flush the output
> buffer if a read() would block.  read_request_line() activates this
> B_SAFEREAD mode while eating CRLFs before a request header.
> 
> This has been very lightly tested.  Emphasis on light.  I just thought I'd
> get it to the list because it sounds like this might be a serious problem.

I think this patch is an awful hack.
If the problem is that some browsers send an extra CRLF after POST data,
then why not explicitly code around this, instead of make the whole of
buff.c harder to maintain.

I suggest that mod_cgi should attempt to read any unwanted CRLF pairs after
having read content-length bytes. To achieve this, you simply need a
routine in buff.c that attempts to read without blocking.

/*
 * Reads up to nbyte bytes of data from the stream without removing data 
 * from the stream. It only returns the data that is either in the
 * user buffer or is available in the OS buffers.
 *
 * Returns the number of bytes available, 0 for EOF or none- vailable,
 * -1 for error.
 */
int
brd_peek(BUFF *fb, void *buf, int nbyte)
{

/*
 1. check for an earlier error on the stream.
 2. If the stream has read-buffering, then copy data out of the buffer into
    buf.
 3. If the buffer is not full, use select() or poll() to 
    test for incoming data, and recv(,,MSG_PEEK) to extract a copy of it.
*/
}

Then mod_cgi simply uses brd_peek() to see if there is any extra data, and
if so it calls bread() to read it.

This routine can also replace any use of btestread() 
brd_peek(fb, buff, 1) > 0  tests for available data.

 David.



> 
> Dean
> 
> Index: buff.c
> ===================================================================
> RCS file: /export/home/cvs/apache/src/buff.c,v
> retrieving revision 1.15
> diff -c -3 -r1.15 buff.c
> *** buff.c	1997/01/25 22:37:10	1.15
> --- buff.c	1997/01/28 10:26:14
> ***************
> *** 175,194 ****
>   }
>   
>   /*
> !  * Set a flag on (1) or off (0). Currently, these flags work
> !  * as a function of the bcwrite() function, so we make sure to
> !  * flush before setting them one way or the other; otherwise
> !  * writes could end up with the wrong flag.
>    */
>   int bsetflag(BUFF *fb, int flag, int value)
>   {
> !     bflush(fb);
>       if (value) fb->flags |= flag;
>       else fb->flags &= ~flag;
>       return value;
>   }
>   
>   /*
>    * Read up to nbyte bytes into buf.
>    * If fewer than byte bytes are currently available, then return those.
>    * Returns 0 for EOF, -1 for error.
> --- 175,243 ----
>   }
>   
>   /*
> !  * Set a flag on (1) or off (0).
>    */
>   int bsetflag(BUFF *fb, int flag, int value)
>   {
> !     if( flag & B_CHUNK ) {
> ! 	/*
> ! 	 * This shouldn't be true for much longer -djg
> ! 	 * Currently, these flags work
> ! 	 * as a function of the bcwrite() function, so we make sure to
> ! 	 * flush before setting them one way or the other; otherwise
> ! 	 * writes could end up with the wrong flag.
> ! 	 */
> ! 	bflush(fb);
> !     }
>       if (value) fb->flags |= flag;
>       else fb->flags &= ~flag;
>       return value;
>   }
>   
>   /*
> +  * return 1 if a read on fd would block, 0 otherwise
> +  */
> + static int
> + read_would_block( int fd )
> + {
> +     fd_set fds;
> +     struct timeval tv;
> +     int rv;
> + 
> +     do {
> + 	FD_ZERO( &fds );
> + 	FD_SET( fd, &fds );
> + 	tv.tv_sec = 0;
> + 	tv.tv_usec = 0;
> + #ifdef HPUX
> + 	rv = select( fd + 1, (int *)&fds, NULL, NULL, &tv );
> + #else
> + 	rv = select( fd + 1, &fds, NULL, NULL, &tv );
> + #endif
> +     } while( rv < 0 && errno == EINTR );
> + 
> +     /* we know for certain that rv == 1 means a read won't block.  rv == 0
> +      * means a read will block.  rv == -1 means some error, treat it as
> +      * if the read will block.
> +      */
> +     return( rv == 1 ? 0 : 1 );
> + }
> + 
> + 
> + /*
> +  * If the read we are about to perform would block then flush the output
> +  * buffer.  This should be called if B_SAFEREAD is set.
> +  */
> + static void
> + bsaferead(BUFF *fb)
> + {
> +     if( read_would_block( fb->fd_in ) ) {
> + 	bflush(fb);
> +     }
> + }
> + 
> + 
> + /*
>    * Read up to nbyte bytes into buf.
>    * If fewer than byte bytes are currently available, then return those.
>    * Returns 0 for EOF, -1 for error.
> ***************
> *** 204,209 ****
> --- 253,259 ----
>       if (!(fb->flags & B_RD))
>       {
>   /* Unbuffered reading */
> + 	if( fb->flags & B_SAFEREAD ) bsaferead(fb);
>   	do i = read(fb->fd_in, buf, nbyte);
>   	while (i == -1 && errno == EINTR);
>   	if (i == -1 && errno != EAGAIN) doerror(fb, B_RD);
> ***************
> *** 233,238 ****
> --- 283,289 ----
>       if (nbyte >= fb->bufsiz)
>       {
>   /* read directly into buffer */
> + 	if( fb->flags & B_SAFEREAD ) bsaferead( fb );
>   	do i = read(fb->fd_in, buf, nbyte);
>   	while (i == -1 && errno == EINTR);
>   	if (i == -1)
> ***************
> *** 248,253 ****
> --- 299,305 ----
>       {
>   /* read into hold buffer, then memcpy */
>   	fb->inptr = fb->inbase;
> + 	if( fb->flags & B_SAFEREAD ) bsaferead( fb );
>   	do i = read(fb->fd_in, fb->inptr, fb->bufsiz);
>   	while (i == -1 && errno == EINTR);
>   	if (i == -1)
> ***************
> *** 310,315 ****
> --- 362,368 ----
>   	    fb->inptr = fb->inbase;
>   	    fb->incnt = 0;
>   	    if (fb->flags & B_EOF) break;
> + 	    if( fb->flags & B_SAFEREAD ) bsaferead( fb );
>   	    do i = read(fb->fd_in, fb->inptr, fb->bufsiz);
>   	    while (i == -1 && errno == EINTR);
>   	    if (i == -1)
> ***************
> *** 381,386 ****
> --- 434,440 ----
>           if (fb->flags & B_EOF)
>               return 0;
>   
> + 	if( fb->flags & B_SAFEREAD ) bsaferead( fb );
>           do {
>               i = read(fb->fd_in, fb->inptr, fb->bufsiz);
>           } while (i == -1 && errno == EINTR);
> ***************
> *** 408,435 ****
>   int
>   btestread(BUFF *fb)
>   {
> -     fd_set fds;
> -     struct timeval tv;
> -     int rv;
> - 
>       /* the simple case, we've already got data in the buffer */
>       if( fb->incnt ) {
>   	return( 0 );
>       }
>   
>       /* otherwise see if the descriptor would block if we try to read it */
> !     do {
> ! 	FD_ZERO( &fds );
> ! 	FD_SET( fb->fd_in, &fds );
> ! 	tv.tv_sec = 0;
> ! 	tv.tv_usec = 0;
> ! #ifdef HPUX
> ! 	rv = select( fb->fd_in + 1, (int *)&fds, NULL, NULL, &tv );
> ! #else
> ! 	rv = select( fb->fd_in + 1, &fds, NULL, NULL, &tv );
> ! #endif
> !     } while( rv < 0 && errno == EINTR );
> !     return( rv == 1 ? 0 : -1 );
>   }
>   
>   /*
> --- 462,474 ----
>   int
>   btestread(BUFF *fb)
>   {
>       /* the simple case, we've already got data in the buffer */
>       if( fb->incnt ) {
>   	return( 0 );
>       }
>   
>       /* otherwise see if the descriptor would block if we try to read it */
> !     return( read_would_block( fb->fd_in ) ? -1 : 0 );
>   }
>   
>   /*
> ***************
> *** 464,469 ****
> --- 503,509 ----
>   	fb->inptr = fb->inbase;
>   	fb->incnt = 0;
>   	if (fb->flags & B_EOF) return 0;
> + 	if( fb->flags & B_SAFEREAD ) bsaferead( fb );
>   	do i = read(fb->fd_in, fb->inptr, fb->bufsiz);
>   	while (i == -1 && errno == EINTR);
>   	if (i == 0) fb->flags |= B_EOF;
> Index: buff.h
> ===================================================================
> RCS file: /export/home/cvs/apache/src/buff.h,v
> retrieving revision 1.10
> diff -c -3 -r1.10 buff.h
> *** buff.h	1997/01/25 22:37:09	1.10
> --- buff.h	1997/01/28 10:26:14
> ***************
> *** 68,73 ****
> --- 68,75 ----
>   #define B_ERROR (48)
>   /* Use chunked writing */
>   #define B_CHUNK (64)
> + /* bflush() if a read would block */
> + #define B_SAFEREAD (128)
>   
>   typedef struct buff_struct BUFF;
>   
> Index: http_protocol.c
> ===================================================================
> RCS file: /export/home/cvs/apache/src/http_protocol.c,v
> retrieving revision 1.93
> diff -c -3 -r1.93 http_protocol.c
> *** http_protocol.c	1997/01/26 01:15:13	1.93
> --- http_protocol.c	1997/01/28 10:26:14
> ***************
> *** 520,530 ****
>       
>       /* Read past empty lines until we get a real request line,
>        * a read error, the connection closes (EOF), or we timeout.
>        */
>       while ((len = getline(l, HUGE_STRING_LEN, conn->client, 0)) <= 0) {
> !         if ((len < 0) || bgetflag(conn->client, B_EOF))
>               return 0;
>       }
>       if (len == (HUGE_STRING_LEN - 1))
>           return 0;               /* Should be a 414 error status instead */
>   
> --- 520,541 ----
>       
>       /* Read past empty lines until we get a real request line,
>        * a read error, the connection closes (EOF), or we timeout.
> +      *
> +      * We skip empty lines because browsers have to tack a CRLF on to the end
> +      * of POSTs to support old CERN webservers.  But we might have mistaken
> +      * this as the client doing a pipelined persistant connection.  So we
> +      * instruct the buffer code to do a bflush() if it would have to block
> +      * during this request, otherwise the client might have to wait until the
> +      * keep alive timeout to get the last segment.
>        */
> +     bsetflag( conn->client, B_SAFEREAD, 1 );
>       while ((len = getline(l, HUGE_STRING_LEN, conn->client, 0)) <= 0) {
> !         if ((len < 0) || bgetflag(conn->client, B_EOF)) {
> ! 	    bsetflag( conn->client, B_SAFEREAD, 0 );
>               return 0;
> + 	}
>       }
> +     bsetflag( conn->client, B_SAFEREAD, 0 );
>       if (len == (HUGE_STRING_LEN - 1))
>           return 0;               /* Should be a 414 error status instead */
>   
> 
> 


Mime
View raw message