--------------------- PatchSet 5978 Date: 2003/10/17 15:13:21 Author: dwsquid Branch: icap-2_5 Tag: (none) Log: Fixes to the MSG_PEEK code. The old code does not handle the case when the ICAP server closes the connection before sending a complete ICAP header. It would just loop there endlessly. The new code always read()s after peek()ing and stores the headers in a MemBuf. If complete ICAP headers are not received, the caller reschedules the read. Members: src/icap_common.c:1.1.2.11->1.1.2.12 src/icap_reqmod.c:1.1.2.4->1.1.2.5 src/icap_respmod.c:1.1.2.7->1.1.2.8 src/protos.h:1.41.6.13.2.11->1.41.6.13.2.12 src/structs.h:1.48.2.9.2.12->1.48.2.9.2.13 Index: squid/src/icap_common.c =================================================================== RCS file: /cvsroot/squid-sf//squid/src/Attic/icap_common.c,v retrieving revision 1.1.2.11 retrieving revision 1.1.2.12 diff -u -r1.1.2.11 -r1.1.2.12 --- squid/src/icap_common.c 17 Oct 2003 15:01:47 -0000 1.1.2.11 +++ squid/src/icap_common.c 17 Oct 2003 15:13:21 -0000 1.1.2.12 @@ -1,5 +1,5 @@ /* - * $Id: icap_common.c,v 1.1.2.11 2003/10/17 15:01:47 dwsquid Exp $ + * $Id: icap_common.c,v 1.1.2.12 2003/10/17 15:13:21 dwsquid Exp $ * * DEBUG: section 81 Internet Content Adaptation Protocol (ICAP) Client * AUTHOR: Geetha Manjunath, Hewlett Packard Company @@ -195,6 +195,7 @@ icap->enc.opt_body = -1; icap->enc.null_body = -1; icap->chunk_size = -1; + memBufDefInit(&icap->icap_hdr); debug(81, 3) ("New ICAP state\n"); return icap; @@ -220,6 +221,8 @@ } requestUnlink(icap->request); icap->request = NULL; + if (!memBufIsNull(&icap->icap_hdr)) + memBufClean(&icap->icap_hdr); if (!memBufIsNull(&icap->respmod.buffer)) memBufClean(&icap->respmod.buffer); if (!memBufIsNull(&icap->respmod.req_hdr_copy)) @@ -338,62 +341,74 @@ } /* - * return 'headlen' + * return: + * -1 if EOF before getting end of ICAP header + * 0 if we don't have the entire ICAP header yet + * 1 if we got the whole header */ int -icapResponsePeek(int fd, IcapStateData * icap, PF reader, int *isIcap) +icapReadHeader(int fd, IcapStateData * icap, int *isIcap) { int headlen = 0; int len = 0; - int read_sz = EXPECTED_ICAP_HEADER_LEN; + int peek_sz = EXPECTED_ICAP_HEADER_LEN; + int read_sz = 0; LOCAL_ARRAY(char, tmpbuf, SQUID_TCP_SO_RCVBUF); - while (!headlen && len >= 0) { - len = recv(fd, tmpbuf, read_sz, MSG_PEEK); + for (;;) { + len = recv(fd, tmpbuf, peek_sz, MSG_PEEK); debug(81, 5) ("recv(FD %d, ..., MSG_PEEK) ret %d\n", fd, len); - if (len > 0) { - headlen = headersEnd(tmpbuf, len); - debug(81, 3) ("headlen=%d\n", headlen); - if (headlen) { - /* End of ICAP header found - Read the \r\n\r\n too */ - len = headlen; - break; - } - if (len == read_sz) { - /* - * The ICAP header is larger than our read buffer, so - * double it and try to peek again. - */ - read_sz *= 2; - assert(read_sz < SQUID_TCP_SO_RCVBUF); - /* XXX return an error instead of the assertion */ - continue; - } - /* - * We don't have all the headers yet, so schedule - * another read. Unfortunately, this causes a fast - * loop until find the end of the headers. We just - * peeked and didn't actually take any data from - * the kernel yet so the socket is still ready for - * reading immediately. - */ - commSetSelect(fd, COMM_SELECT_READ, reader, icap, 0); - commSetTimeout(fd, Config.Timeout.read, icapConnectTimeout, icap); + if (len < 0) { + debug(81, 1) ("icapReadHeader: FD %d recv error: %s\n", fd, xstrerror()); return -1; } - if (!len && !headlen) { - /* No data */ - commSetSelect(fd, COMM_SELECT_READ, reader, icap, 0); - commSetTimeout(fd, Config.Timeout.read, icapConnectTimeout, icap); + if (len == 0) { + debug(81, 1) ("icapReadHeader: FD %d recv EOF\n", fd); return -1; } - } - if (headlen < 4) - *isIcap = 0; - else if (0 == strncmp(tmpbuf, "ICAP", 4)) - *isIcap = 1; + headlen = headersEnd(tmpbuf, len); + debug(81, 3) ("headlen=%d\n", headlen); + /* + * break if we now know where the ICAP headers end + */ + if (headlen) + break; + /* + * break if we know there is no more data to read + */ + if (len < peek_sz) + break; + /* + * The ICAP header is larger than (or equal to) our read + * buffer, so double it and try to peek again. + */ + peek_sz *= 2; + assert(peek_sz < SQUID_TCP_SO_RCVBUF); + /* XXX return an error instead of the assertion */ + } + /* + * Now actually read the data from the kernel + */ + if (headlen) + read_sz = headlen; else - *isIcap = 0; - return headlen; + read_sz = len; + len = read(fd, tmpbuf, read_sz); + assert(len == read_sz); + memBufAppend(&icap->icap_hdr, tmpbuf, len); + if (headlen) { + /* End of ICAP header found */ + if (icap->icap_hdr.size < 4) + *isIcap = 0; + else if (0 == strncmp(icap->icap_hdr.buf, "ICAP", 4)) + *isIcap = 1; + else + *isIcap = 0; + return 1; + } + /* + * We don't have all the headers yet + */ + return 0; } static int Index: squid/src/icap_reqmod.c =================================================================== RCS file: /cvsroot/squid-sf//squid/src/Attic/icap_reqmod.c,v retrieving revision 1.1.2.4 retrieving revision 1.1.2.5 diff -u -r1.1.2.4 -r1.1.2.5 --- squid/src/icap_reqmod.c 17 Oct 2003 15:01:48 -0000 1.1.2.4 +++ squid/src/icap_reqmod.c 17 Oct 2003 15:13:21 -0000 1.1.2.5 @@ -1,5 +1,5 @@ /* - * $Id: icap_reqmod.c,v 1.1.2.4 2003/10/17 15:01:48 dwsquid Exp $ + * $Id: icap_reqmod.c,v 1.1.2.5 2003/10/17 15:13:21 dwsquid Exp $ * * DEBUG: section 81 Internet Content Adaptation Protocol (ICAP) Client * AUTHOR: Geetha Manjunath, Hewlett Packard Company @@ -298,9 +298,8 @@ icapReqModReadIcapPart(int fd, void *data) { IcapStateData *icap = data; - LOCAL_ARRAY(char, icap_header, SQUID_TCP_SO_RCVBUF); LOCAL_ARRAY(char, tmpbuf, SQUID_TCP_SO_RCVBUF); - int headlen; + int x; const char *start; const char *end; int status; @@ -311,68 +310,51 @@ debug(81, 5) ("icapReqModReadIcapPart: FD %d httpState = %p\n", fd, data); statCounter.syscalls.sock.reads++; - headlen = icapResponsePeek(fd, icap, icapReqModReadIcapPart, &isIcap); - debug(81, 3) ("icapReqModReadIcapPart: headlen=%d, isIcap=%d\n", - headlen, isIcap); - if (headlen < 0) + x = icapReadHeader(fd, icap, &isIcap); + if (x < 0) { + /* Did not find a proper ICAP response */ + debug(81, 3) ("ICAP : Error path!\n"); + icapEntryError(icap, ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR, errno); + comm_close(fd); return; - - if (!isIcap && headlen) { - /* Could be sending the HTTP reply directly - bug but still allow :-) */ - if (strstr(tmpbuf, "HTTP")) - directResponse = 1; - } else { + } + if (x == 0) { /* - * Read the ICAP header only - leave the rest + * Waiting for more headers. Schedule new read hander, but + * don't reset timeout. */ - ssize_t rl = read(fd, icap_header, headlen); - - debug(81, 3) ("icapReqModReadIcapPart: headlen=%d, rl=%d\n", headlen, rl); - if (0 == headlen || rl != headlen) { - /* - * Did not find a proper ICAP response - */ - debug(81, 3) - ("ICAP Error : Did not find a proper ICAP response at line %d!\n", - __LINE__); - icapEntryError(icap, - ERR_READ_ERROR, - HTTP_INTERNAL_SERVER_ERROR, - errno); - comm_close(fd); - return; - } - debug(81, 5) ("icapReqModReadIcapPart: read() returns %d\n", headlen); - icap_header[headlen] = '\0'; - debug(81, 3) ("Read icap header : <%s>\n", icap_header); - ver = -999.999; /* initalize the version to a bogus number. I + commSetSelect(fd, COMM_SELECT_READ, icapReqModReadIcapPart, icap, 0); + return; + } + /* + * Parse the ICAP header + */ + assert(icap->icap_hdr.size); + debug(81, 3) ("Read icap header : <%s>\n", icap->icap_hdr.buf); + ver = -999.999; /* initalize the version to a bogus number. I * think that we should parse it using 2 * integers and a %d.%d scanf format - Basile * june 2002 */ - if (sscanf(icap_header, "ICAP/%f %d %s\r", &ver, &status, tmpbuf) < 3 - || ver <= 0.0) { - debug(81, 1) ("BAD ICAP status line <%s>\n", icap_header); - /* is this correct in case of ICAP protocol error? */ - icapEntryError(icap, - ERR_READ_ERROR, - HTTP_INTERNAL_SERVER_ERROR, - errno); - comm_close(fd); - return; - }; - icapSetKeepAlive(icap, icap_header); - if (icapFindHeader(icap_header, "Encapsulated:", &start, &end)) { - icapParseEncapsulated(icap, start, end); - } else { - debug(81, 1) ("WARNING: icapReqModReadIcapPart() did not find 'Encapsulated' header\n"); - } - if (icap->enc.res_hdr > -1) - directResponse = 1; - else if (icap->enc.res_body > -1) - directResponse = 1; - else - directResponse = 0; + if (sscanf(icap->icap_hdr.buf, "ICAP/%f %d %s\r", &ver, &status, tmpbuf) < 3 + || ver <= 0.0) { + debug(81, 1) ("BAD ICAP status line <%s>\n", icap->icap_hdr.buf); + /* is this correct in case of ICAP protocol error? */ + icapEntryError(icap, ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR, errno); + comm_close(fd); + return; + }; + icapSetKeepAlive(icap, icap->icap_hdr.buf); + if (icapFindHeader(icap->icap_hdr.buf, "Encapsulated:", &start, &end)) { + icapParseEncapsulated(icap, start, end); + } else { + debug(81, 1) ("WARNING: icapReqModReadIcapPart() did not find 'Encapsulated' header\n"); } + if (icap->enc.res_hdr > -1) + directResponse = 1; + else if (icap->enc.res_body > -1) + directResponse = 1; + else + directResponse = 0; debug(81, 3) ("icapReqModReadIcapPart: directResponse=%d\n", directResponse); /* Check whether it is a direct reply - if so over to http part */ Index: squid/src/icap_respmod.c =================================================================== RCS file: /cvsroot/squid-sf//squid/src/Attic/icap_respmod.c,v retrieving revision 1.1.2.7 retrieving revision 1.1.2.8 diff -u -r1.1.2.7 -r1.1.2.8 --- squid/src/icap_respmod.c 17 Oct 2003 15:01:48 -0000 1.1.2.7 +++ squid/src/icap_respmod.c 17 Oct 2003 15:13:21 -0000 1.1.2.8 @@ -1,5 +1,5 @@ /* - * $Id: icap_respmod.c,v 1.1.2.7 2003/10/17 15:01:48 dwsquid Exp $ + * $Id: icap_respmod.c,v 1.1.2.8 2003/10/17 15:13:21 dwsquid Exp $ * * DEBUG: section 81 Internet Content Adaptation Protocol (ICAP) Client * AUTHOR: Geetha Manjunath, Hewlett Packard Company @@ -289,10 +289,8 @@ icapRespModReadReply(int fd, void *data) { IcapStateData *icap = data; - LOCAL_ARRAY(char, icap_header, SQUID_TCP_SO_RCVBUF); LOCAL_ARRAY(char, tmpbuf, SQUID_TCP_SO_RCVBUF); - int len; - int headlen; + int x; int status = 0; int isIcap = 0; int directResponse = 0; @@ -308,125 +306,118 @@ /* Else it will loop */ icap->request->flags.do_icap = 0; - headlen = icapResponsePeek(fd, icap, icapRespModReadReply, &isIcap); - if (headlen < 0) + x = icapReadHeader(fd, icap, &isIcap); + if (x < 0) { + /* Did not find a proper ICAP response */ + debug(81, 3) ("ICAP : Error path!\n"); + err = errorCon(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR); + err->request = requestLink(icap->request); + err->xerrno = errno; + errorAppendEntry(icap->respmod.entry, err); + comm_close(fd); return; - - if (!isIcap && headlen) { - /* Could be sending the HTTP reply directly without ICAP header - bug - * but still allow :-) */ - if (strstr(tmpbuf, "HTTP")) - directResponse = 1; - } else { + } + if (x == 0) { /* - * Read the ICAP header only - leave the rest + * Waiting for more headers. Schedule new read hander, but + * don't reset timeout. */ - len = read(fd, icap_header, headlen); - debug(81, 5) ("icapRespModReadReply: read() returns %d\n", len); - if (len > 0) { - icap_header[headlen] = '\0'; - debug(81, 3) ("Read icap header : <%s>\n", icap_header); - ver = -999.999; /* initalize the version to a bogus number. I + commSetSelect(fd, COMM_SELECT_READ, icapRespModReadReply, icap, 0); + return; + } + /* + * Parse the ICAP header + */ + assert(icap->icap_hdr.size); + debug(81, 3) ("Parse icap header : <%s>\n", icap->icap_hdr.buf); + ver = -999.999; /* initalize the version to a bogus number. I * think that we should parse it using 2 * integers and a %d.%d scanf format - Basile * june 2002 */ - if (sscanf(icap_header, "ICAP/%f %d %s\r", &ver, &status, - tmpbuf) < 3 || ver <= 0.0) { - debug(81, 1) ("BAD ICAP status line <%s>\n", icap_header); - /* is this correct in case of ICAP protocol error? */ - err = errorCon(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR); - err->request = requestLink(icap->request); - err->xerrno = errno; - errorAppendEntry(icap->respmod.entry, err); - comm_close(fd); - return; - }; - } - if (icap->flags.wait_for_preview_reply) { - if (status == 100) { - debug(81, 5) ("icapRespModReadReply: 100 Continue received\n"); - icap->flags.wait_for_preview_reply = 0; - /* - * call again icapSendRespMod to handle data that - * was received while waiting fot this ICAP response - */ - icapSendRespMod(icap, NULL, 0, 0); - return; -#if SUPPORT_ICAP_204 - } else if (status == 204) { - if (icap->flags.http_server_eof) { - /* Reset is required to avoid duplicate stmemFreeDataUpto , - * but will I loose all info now ? */ - /* storeEntryReset(icap->respmod.entry); */ - /* stmemFreeDataUpto(&(entry->mem_obj->data_hdr), -icap->sc); */ - fwdComplete(httpState->fwd); - } else { - commSetSelect(fd, COMM_SELECT_READ, httpReadReply, - icap, 0); - } - comm_close(fd); - return; -#endif - } - } - if (!headlen || len != headlen) { - /* Did not find a proper ICAP response */ - debug(81, 3) ("ICAP : Error path!\n"); - err = errorCon(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR); - err->request = requestLink(icap->request); - err->xerrno = errno; - errorAppendEntry(icap->respmod.entry, err); - comm_close(fd); - return; - } - if (100 == status) { - debug(81, 3) ("icapRespModReadReply: 100 Continue received\n"); - commSetSelect(fd, COMM_SELECT_READ, icapRespModReadReply, icap, 0); - return; - } - icapSetKeepAlive(icap, icap_header); - if (204 == status) { - debug(81, 3) ("got 204 status from ICAP server\n"); - debug(81, 3) ("setting icap->flags.no_content\n"); - icap->flags.no_content = 1; - /* - * copy the response already written to the ICAP server - */ - debug(81, 3) ("copying %d bytes from resp_copy to entry\n", - icap->respmod.resp_copy.size); - if (icapReadReply2(icap, - icap->respmod.resp_copy.buf, - icap->respmod.resp_copy.size) < 0) - comm_close(fd); + if (sscanf(icap->icap_hdr.buf, "ICAP/%f %d %s\r", &ver, &status, tmpbuf) < 3 || ver <= 0.0) { + debug(81, 1) ("BAD ICAP status line <%s>\n", icap->icap_hdr.buf); + /* is this correct in case of ICAP protocol error? */ + err = errorCon(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR); + err->request = requestLink(icap->request); + err->xerrno = errno; + errorAppendEntry(icap->respmod.entry, err); + comm_close(fd); + return; + }; + if (icap->flags.wait_for_preview_reply) { + if (status == 100) { + debug(81, 5) ("icapRespModReadReply: 100 Continue received\n"); + icap->flags.wait_for_preview_reply = 0; /* - * XXX ideally want to clean icap->respmod.resp_copy here - * XXX ideally want to "close" ICAP server connection here + * call again icapSendRespMod to handle data that + * was received while waiting fot this ICAP response */ + icapSendRespMod(icap, NULL, 0, 0); return; - } - icap->flags.copy_response = 0; - if (200 != status) { - debug(81, 1) ("Unsupported status '%d' from ICAP server\n", status); - /* Did not find a proper ICAP response */ - err = errorCon(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR); - err->request = requestLink(icap->request); - err->xerrno = errno; - errorAppendEntry(icap->respmod.entry, err); +#if SUPPORT_ICAP_204 + } else if (status == 204) { + if (icap->flags.http_server_eof) { + /* Reset is required to avoid duplicate stmemFreeDataUpto , + * but will I loose all info now ? */ + /* storeEntryReset(icap->respmod.entry); */ + /* stmemFreeDataUpto(&(entry->mem_obj->data_hdr), -icap->sc); */ + fwdComplete(httpState->fwd); + } else { + commSetSelect(fd, COMM_SELECT_READ, httpReadReply, + icap, 0); + } comm_close(fd); return; +#endif } - if (icapFindHeader(icap_header, "Encapsulated:", &start, &end)) { - icapParseEncapsulated(icap, start, end); - } else { - debug(81, 1) ("WARNING: icapReqModReadReply() did not find 'Encapsulated' header\n"); - } - if (icap->enc.res_hdr > -1) - directResponse = 1; - else if (icap->enc.res_body > -1) - directResponse = 1; - else - directResponse = 0; } + if (100 == status) { + debug(81, 3) ("icapRespModReadReply: 100 Continue received\n"); + commSetSelect(fd, COMM_SELECT_READ, icapRespModReadReply, icap, 0); + return; + } + icapSetKeepAlive(icap, icap->icap_hdr.buf); + if (204 == status) { + debug(81, 3) ("got 204 status from ICAP server\n"); + debug(81, 3) ("setting icap->flags.no_content\n"); + icap->flags.no_content = 1; + /* + * copy the response already written to the ICAP server + */ + debug(81, 3) ("copying %d bytes from resp_copy to entry\n", + icap->respmod.resp_copy.size); + if (icapReadReply2(icap, + icap->respmod.resp_copy.buf, + icap->respmod.resp_copy.size) < 0) + comm_close(fd); + /* + * XXX ideally want to clean icap->respmod.resp_copy here + * XXX ideally want to "close" ICAP server connection here + */ + return; + } + icap->flags.copy_response = 0; + if (200 != status) { + debug(81, 1) ("Unsupported status '%d' from ICAP server\n", status); + /* Did not find a proper ICAP response */ + err = errorCon(ERR_READ_ERROR, HTTP_INTERNAL_SERVER_ERROR); + err->request = requestLink(icap->request); + err->xerrno = errno; + errorAppendEntry(icap->respmod.entry, err); + comm_close(fd); + return; + } + if (icapFindHeader(icap->icap_hdr.buf, "Encapsulated:", &start, &end)) { + icapParseEncapsulated(icap, start, end); + } else { + debug(81, 1) ("WARNING: icapReqModReadReply() did not find 'Encapsulated' header\n"); + } + if (icap->enc.res_hdr > -1) + directResponse = 1; + else if (icap->enc.res_body > -1) + directResponse = 1; + else + directResponse = 0; /* * "directResponse" is the normal case here. If we don't have @@ -517,6 +508,9 @@ /* Schedule reading the ICAP response */ debug(81, 3) ("icapSendRespModDone: FD %d: commSetSelect on read icapRespModReadReply.\n", fd); commSetSelect(fd, COMM_SELECT_READ, icapRespModReadReply, icap, 0); +#if 1 + commSetTimeout(fd, Config.Timeout.read, icapReadTimeout, icap); +#else if (icap->flags.wait_for_preview_reply || icap->flags.http_server_eof) { /* * Set the read timeout only after all data has been sent @@ -527,6 +521,7 @@ */ commSetTimeout(fd, Config.Timeout.read, icapReadTimeout, icap); } +#endif } } Index: squid/src/protos.h =================================================================== RCS file: /cvsroot/squid-sf//squid/src/protos.h,v retrieving revision 1.41.6.13.2.11 retrieving revision 1.41.6.13.2.12 diff -u -r1.41.6.13.2.11 -r1.41.6.13.2.12 --- squid/src/protos.h 17 Oct 2003 15:01:48 -0000 1.41.6.13.2.11 +++ squid/src/protos.h 17 Oct 2003 15:13:21 -0000 1.41.6.13.2.12 @@ -1,6 +1,6 @@ /* - * $Id: protos.h,v 1.41.6.13.2.11 2003/10/17 15:01:48 dwsquid Exp $ + * $Id: protos.h,v 1.41.6.13.2.12 2003/10/17 15:13:21 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -1362,7 +1362,7 @@ const char *icapServiceToStr(const icap_service_t); int icapCheckAcl(clientHttpRequest *); size_t icapLineLength(const char *, int); -int icapResponsePeek(int, IcapStateData *, PF, int *); +int icapReadHeader(int, IcapStateData *, int *); int icapFindHeader(const char *, const char *, const char **, const char **); int icapParseKeepAlive(const IcapStateData *, const char *, const char *); void icapSetKeepAlive(IcapStateData * icap, const char *hdrs); Index: squid/src/structs.h =================================================================== RCS file: /cvsroot/squid-sf//squid/src/structs.h,v retrieving revision 1.48.2.9.2.12 retrieving revision 1.48.2.9.2.13 diff -u -r1.48.2.9.2.12 -r1.48.2.9.2.13 --- squid/src/structs.h 24 Sep 2003 19:35:02 -0000 1.48.2.9.2.12 +++ squid/src/structs.h 17 Oct 2003 15:13:22 -0000 1.48.2.9.2.13 @@ -1,6 +1,6 @@ /* - * $Id: structs.h,v 1.48.2.9.2.12 2003/09/24 19:35:02 dwsquid Exp $ + * $Id: structs.h,v 1.48.2.9.2.13 2003/10/17 15:13:22 dwsquid Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -993,6 +993,7 @@ int icap_fd; int sc; icap_service *current_service; + MemBuf icap_hdr; struct { int res_hdr; int res_body;