multipart boundary separation totally broken with runs of <CR>s; I must approach this differently.

4 files changed, 84 insertions, 97 deletions

diff --git a/picohttp.c b/picohttp.c
index 9b28b92..7b3dfbc 100644
--- a/picohttp.c
+++ b/picohttp.c
@@ -453,7 +453,7 @@ static size_t picohttpMatchURL(
 	return j;
 }
 
-static int8_t picohttpMatchRoute(
+static int picohttpMatchRoute(
 	struct picohttpRequest * const req,
 	struct picohttpURLRoute const * const routes )
 {
@@ -1113,6 +1113,25 @@ int picohttpResponseWrite (
 	return len;
 }
 
+static void picohttpMultipartSync(
+	struct picohttpMultipart * const mp)
+{
+	if( '\r' == mp->req->query.prev_ch[0] ) {
+		mp->replayhead = 0; 
+		mp->in_boundary = 1;
+		debug_printf("mpsync <CR>\n");
+	} else
+	if( '\n' == mp->req->query.prev_ch[0] &&
+	    '\r' == mp->req->query.prev_ch[1] ) {
+		mp->replayhead = 1;
+		mp->in_boundary = 2;
+		debug_printf("mpsync <CR><LF>\n");
+	} else {
+		mp->replayhead = -1;
+		mp->in_boundary = 0;
+	}
+}
+
 int picohttpMultipartGetch(
 	struct picohttpMultipart * const mp)
 {
@@ -1124,18 +1143,17 @@ int picohttpMultipartGetch(
 		if( mp->replayhead < mp->replay ) {
 			ch = mp->req->query.multipartboundary[mp->replayhead];
 			mp->replayhead++;
-			debug_printf("replay head: %0.2x\n", ch);
+			debug_printf(" >> : %0.2x\n", ch);
 			return ch;
 		} else {
-			ch = mp->req->query.prev_ch[0];
 			mp->replay = 0;
-			mp->replayhead = 0;
-			debug_printf("replay prev: %0.2x\n", ch);
+			ch = mp->req->query.prev_ch[0];
+			picohttpMultipartSync(mp);
+			debug_printf(" >>|: %0.2x\n", ch);
 			return ch;
 		}
 	} else {
-		mp->replay = 0;
-
+end_of_replay_was_CR:
 		ch = picohttpGetch(mp->req);
 
 	/* picohttp's query and header parsing is forgiving
@@ -1145,40 +1163,17 @@ int picohttpMultipartGetch(
 	 * a <CR><LF> sequence.
 	 */
 
-		switch(ch) {
-		case '\r':
-			debug_printf("(CR)", ch); break;
-		case '\n':
-			debug_printf("(LF)", ch); break;
-		default:
-			debug_printf("(%c)", ch);
-		}
-		if( '\r' == ch ) {
-			mp->replayhead =
-			mp->in_boundary = 0;
-			if( '\r' == mp->req->query.prev_ch[1] ) {
-				debug_printf("<CR|run>");
-				return '\r';
-			} else {
-				debug_printf("<CR>");
+		while( 0 <= ch ) {
+
+			switch(ch) {
+			case '\r':
+				debug_printf("(CR)", ch); break;
+			case '\n':
+				debug_printf("(LF)", ch); break;
+			default:
+				debug_printf("(%c)", ch);
 			}
-		} else
-		if( '\n' == ch &&
-		    '\r' == mp->req->query.prev_ch[1] ) {
-			mp->replayhead = 
-			mp->in_boundary = 1;
-			debug_printf("<CR><LF>");
-		} else
-		if(  '-' == ch &&
-		    '\n' == mp->req->query.prev_ch[1] &&
-		    '\r' == mp->req->query.prev_ch[2]
-		) {
-			mp->replayhead =
-			mp->in_boundary = 2;
-			debug_printf("<CR><LF>-");
-		} 
 
-		while( 0 <= ch ) {
 
 			if( mp->req->query.multipartboundary[mp->in_boundary] == ch ) {
 				if( 0 == mp->req->query.multipartboundary[mp->in_boundary+1] ) {
@@ -1197,9 +1192,10 @@ int picohttpMultipartGetch(
 						mp->finished = 1;
 					}
 
-				/* TODO: Technically the last boundary is followed by
-				 * a last <CR><LF> sequence... we should check for this
-				 * as well, just for completeness */
+				/* TODO: Technically the last boundary is followed by a
+				 * terminating <CR><LF> sequence... we should check for
+				 * this as well, just for completeness
+				 */
 					if(trail[0] == '-' && trail[1] == '-')
 						mp->finished = 2;
 
@@ -1208,55 +1204,22 @@ int picohttpMultipartGetch(
 				debug_printf("{#}", ch);
 				mp->in_boundary++;
 			} else {
-				debug_printf("{_}", ch);
+				debug_printf("{_|%0.2x", ch);
 				if( mp->in_boundary ) {
-
-				/* In case the mismatch was due to a <CR> or <LF>
-				 * or '-' character, it must be checked, if this may be
-				 * preceeded by some <CR><LF>* sequence that would
-				 * allow to be a valid multipart boundary.
-				 * In that case the exact replay parameters depend
-				 * on the specific combination.
-				 *
-				 * The replay code above also emits the last character
-				 * read from IO, but in this case that character is
-				 * far ahead of what should actually be returned.
-				 * So we maniulate the content of the prev_ch[0]
-				 * to be the final character of the replay and
-				 * have the replay from the boundary buffer be one
-				 * position short
-				 */
-
-				/* dw: I really hate how deep this nests, but
-				 * unfortunately HTTP and multipart body parsing
-				 * is a nasty, convoluted state machine
-				 */
+					mp->replay = mp->in_boundary;
 					if( '\r' == ch ) {
-						mp->replay = mp->in_boundary - 1;
+						mp->replayhead = 0;
 						mp->in_boundary = 1;
-						mp->req->query.prev_ch[0] = 
-							mp->req->query.multipartboundary[mp->replay];
-					} else
-					if( '\n' == ch &&
-					    '\r' == mp->req->query.prev_ch[1] ) {
-						mp->replay = mp->in_boundary - 2;
-						mp->in_boundary = 2;
-						mp->req->query.prev_ch[0] = 
-							mp->req->query.multipartboundary[mp->replay];
-					} else
-					if(  '-' == ch &&
-					    '\n' == mp->req->query.prev_ch[1] &&
-					    '\r' == mp->req->query.prev_ch[2]
-					) {
-						mp->replay = mp->in_boundary - 4;
-						mp->in_boundary = 2;
 					} else {
-						mp->replay = mp->in_boundary;
+						mp->replayhead = 1;
 						mp->in_boundary = 0;
 					}
-
 					ch = mp->req->query.multipartboundary[mp->replayhead++];
+
+					debug_printf("|%d..%d", mp->replayhead, mp->replay);
 				}
+				debug_putc('}');
+
 				return ch;
 			}
 			ch = picohttpGetch(mp->req);
@@ -1368,6 +1331,8 @@ struct picohttpMultipart picohttpMultipartStart(
 		.replayhead = 0
 	};
 
+	picohttpMultipartSync(&mp);
+
 	return mp;
 }
 
diff --git a/picohttp.h b/picohttp.h
index 380584c..d3931ab 100644
--- a/picohttp.h
+++ b/picohttp.h
@@ -47,10 +47,10 @@
 #define PICOHTTP_STATUS_505_HTTP_VERSION_NOT_SUPPORTED 505
 
 struct picohttpIoOps {
-	int (*read)(size_t /*count*/, char* /*buf*/, void*);
-	int (*write)(size_t /*count*/, char const* /*buf*/, void*);
+	int (*read)(size_t /*count*/, void* /*buf*/, void*);
+	int (*write)(size_t /*count*/, void const* /*buf*/, void*);
 	int (*getch)(void*); // returns negative value on error
-	int (*putch)(char, void*);
+	int (*putch)(int, void*);
 	int (*flush)(void*);
 	void *data;
 };
@@ -120,8 +120,8 @@ struct picohttpRequest {
 		size_t contentlength;
 		uint8_t contentencoding;
 		uint8_t transferencoding;
-		char multipartboundary[PICOHTTP_MULTIPARTBOUNDARY_MAX_LEN+1];
-		char prev_ch[5];
+		unsigned char multipartboundary[PICOHTTP_MULTIPARTBOUNDARY_MAX_LEN+1];
+		unsigned char prev_ch[5];
 		size_t chunklength;
 	} query;
 	struct {
diff --git a/test/Makefile b/test/Makefile
index 2a831d4..e96cb0f 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -1,2 +1,9 @@
-bsdsocket: bsdsocket.c ../picohttp.c
+.PHONY: all
+
+all: bsdsocket bsdsocket_nhd
+
+bsdsocket: bsdsocket.c ../picohttp.c ../picohttp.h
 	$(CC) -std=c99 -DHOST_DEBUG -O0 -g3 -I../ -o bsdsocket ../picohttp.c bsdsocket.c
+	
+bsdsocket_nhd: bsdsocket.c ../picohttp.c ../picohttp.h
+	$(CC) -std=c99 -O0 -g3 -I../ -o bsdsocket_nhd ../picohttp.c bsdsocket.c
diff --git a/test/bsdsocket.c b/test/bsdsocket.c
index 1d5f8d5..5350db9 100644
--- a/test/bsdsocket.c
+++ b/test/bsdsocket.c
@@ -15,14 +15,14 @@
 
 #include "../picohttp.h"
 
-int bsdsock_read(size_t count, char *buf, void *data)
+int bsdsock_read(size_t count, void *buf, void *data)
 {
 	int fd = *((int*)data);
 	
 	ssize_t rb = 0;
 	ssize_t r = 0;
 	do {
-		r = read(fd, buf+rb, count-rb);
+		r = read(fd, (unsigned char*)buf + rb, count-rb);
 		if( 0 < r ) {
 			rb += r;
 			continue;
@@ -41,14 +41,14 @@ int bsdsock_read(size_t count, char *buf, void *data)
 	return rb;
 }
 
-int bsdsock_write(size_t count, char const *buf, void *data)
+int bsdsock_write(size_t count, void const *buf, void *data)
 {
 	int fd = *((int*)data);
 	
 	ssize_t wb = 0;
 	ssize_t w = 0;
 	do {
-		w = write(fd, buf+wb, count-wb);
+		w = write(fd, (unsigned char*)buf + wb, count-wb);
 		if( 0 < w ) {
 			wb += w;
 			continue;
@@ -69,16 +69,17 @@ int bsdsock_write(size_t count, char const *buf, void *data)
 
 int bsdsock_getch(void *data)
 {
-	char ch;
+	unsigned char ch;
 	int err;
 	if( 1 != (err = bsdsock_read(1, &ch, data)) )
 		return err;
 	return ch;
 }
 
-int bsdsock_putch(char ch, void *data)
+int bsdsock_putch(int ch, void *data)
 {
-	return bsdsock_write(1, &ch, data);
+	char ch_ = ch;
+	return bsdsock_write(1, &ch_, data);
 }
 
 int bsdsock_flush(void* data)
@@ -144,11 +145,21 @@ void rhUpload(struct picohttpRequest *req)
 
 	struct picohttpMultipart mp = picohttpMultipartStart(req);
 	
+	chdir("/tmp/uploadtest");
 	while( !picohttpMultipartNext(&mp) ) {
 		fprintf(stderr, "\nprocessing form field \"%s\"\n", mp.disposition.name);
-		for(int16_t ch = picohttpMultipartGetch(&mp);
+		FILE *fil = fopen(mp.disposition.name, "wb");
+		if(!fil) {
+			continue;
+		}
+
+		for(int ch = picohttpMultipartGetch(&mp);
 		    0 <= ch;
 		    ch = picohttpMultipartGetch(&mp) ) {
+			fputc(ch, fil);
+
+		#if HOST_DEBUG
+			fputs("\e[32m", stderr);
 			switch(ch) {
 			case '\r':
 				fputs("[CR]", stderr); break;
@@ -158,7 +169,11 @@ void rhUpload(struct picohttpRequest *req)
 			default:
 				fputc(ch, stderr);
 			}
+			fputs("\e[0m", stderr);
+		#endif/*HOST_DEBUG*/
 		}
+
+		fclose(fil);
 		if( !mp.finished ) {
 			break;
 		}