Revision: 14535
Author: adrian.chadd
Date: Mon Apr 5 00:40:47 2010
Log: Issue #99 fixes - fix method_t pointer quirkiness
This commit modifies the behaviour of requestCreate() and code which
relies on copying method_t pointers around using urlMethodAssign().
Since urlMethodAssign() now may copy a method, rather than "gifting"
it to the called function, the behaviour of callers needed some
tinkering. In this specific case, parseHttpRequest() now needs to
call urlMethodFree() on the created method after parsing it.
In the past it would be just gifted to the request_t created somewhere
else; but urlMethodAssign() will now cheerfully create a private
duplicate just for that request.
For correctness, src/htcp.c also now explicitly calls urlMethodFree().
All calls to urlMethodGet*() should be followed by an explicit
urlMethodFree() now and I'll hopefully do this in some subsequent
commits.
urlMethodAssign() will now:
* free the previous destination pointer if it's set and if it's a
METHOD_OTHER pointer;
* call urlMethodDup() on the src pointer rather than blindly copying
the value over.
This modifies two things:
* If the assignment would overwrite a method_t pointer that is pointing
do a METHOD_OTHER method, it'll first free it, preventing memory leaks;
* If the assignment is from a METHOD_OTHER method, it creates a new method
and assigns that rather than point to the existing one.
This fixes the specific crash in Issue #99 where a method_t pointer,
pointing
to a METHOD_OTHER method, is free'd before the method_t pointer is used
in the final request logging bit.
The performance and memory allocation use in the standard method case is
the same. There'll be a very slight increase in memory use when handling
non-normal methods.
http://code.google.com/p/lusca-cache/source/detail?r=14535
Modified:
/branches/LUSCA_HEAD/libhttp/HttpMethod.c
/branches/LUSCA_HEAD/src/client_side.c
/branches/LUSCA_HEAD/src/client_side_request_parse.c
/branches/LUSCA_HEAD/src/htcp.c
=======================================
--- /branches/LUSCA_HEAD/libhttp/HttpMethod.c Fri Apr 2 02:19:00 2010
+++ /branches/LUSCA_HEAD/libhttp/HttpMethod.c Mon Apr 5 00:40:47 2010
@@ -169,8 +169,10 @@
debug(23, 1) ("urlMethodAssign: overwriting an existing method:
'%s'\n",
urlMethodGetConstStr((*dst)));
}
-
- (*dst) = src;
+ if (*dst)
+ urlMethodFree(*dst);
+
+ (*dst) = urlMethodDup(src);
}
method_t *
=======================================
--- /branches/LUSCA_HEAD/src/client_side.c Fri Apr 2 02:06:29 2010
+++ /branches/LUSCA_HEAD/src/client_side.c Mon Apr 5 00:40:47 2010
@@ -129,6 +129,12 @@
EBIT_TEST(r->cache_control->mask, CC_ONLY_IF_CACHED);
}
+/*
+ * Create a store entry for the given request information.
+ *
+ * the method_t passed in is not "given" to the request; it is still
+ * the responsibility of the caller to free.
+ */
StoreEntry *
clientCreateStoreEntry(clientHttpRequest * h, method_t * m, request_flags
flags)
{
=======================================
--- /branches/LUSCA_HEAD/src/client_side_request_parse.c Sat Mar 20
18:59:26 2010
+++ /branches/LUSCA_HEAD/src/client_side_request_parse.c Mon Apr 5
00:40:47 2010
@@ -140,6 +140,12 @@
return 0;
}
+/*
+ * Abort the current request during parsing.
+ *
+ * If the method pointer is set, leave it. Otherwise, set it to
+ * METHOD_NONE so it's set to -something-.
+ */
static clientHttpRequest *
parseHttpRequestAbort(ConnStateData * conn, method_t ** method_p, const
char *uri)
{
@@ -280,6 +286,8 @@
* Returns
* NULL on error or incomplete request
* a clientHttpRequest structure on success
+ * method_p may be set to a parsed method, or METHOD_NONE, or NULL.
+ * It's the callers' responsibility to transfer or free the method.
*/
static clientHttpRequest *
parseHttpRequest(ConnStateData * conn, HttpMsgBuf * hmsg, method_t **
method_p, int *status)
@@ -429,12 +437,12 @@
int nrequests;
dlink_node *n;
clientHttpRequest *http = NULL;
- method_t *method;
+ method_t *method = NULL;
ErrorState *err = NULL;
int parser_return_code = 0;
request_t *request = NULL;
HttpMsgBuf msg;
-
+ int ret = -1;
/* Skip leading (and trailing) whitespace */
while (conn->in.offset > 0 && xisspace(conn->in.buf[0])) {
@@ -442,8 +450,10 @@
conn->in.offset--;
}
conn->in.buf[conn->in.offset] = '\0'; /* Terminate the string */
- if (conn->in.offset == 0)
- return 0;
+ if (conn->in.offset == 0) {
+ ret = 0;
+ goto finish;
+ }
HttpMsgBufInit(&msg, conn->in.buf, conn->in.offset); /* XXX for now
there's no deallocation function needed but this may change */
/* Limit the number of concurrent requests to 2 */
@@ -452,7 +462,8 @@
debug(33, 3) ("clientTryParseRequest: FD %d max concurrent requests
reached\n", fd);
debug(33, 5) ("clientTryParseRequest: FD %d defering new request until
one is done\n", fd);
conn->defer.until = squid_curtime + 100; /* Reset when a request is
complete */
- return 0;
+ ret = 0;
+ goto finish;
}
conn->in.buf[conn->in.offset] = '\0'; /* Terminate the string */
if (nrequests == 0)
@@ -476,7 +487,8 @@
http->log_type = LOG_TCP_DENIED;
http->entry = clientCreateStoreEntry(http, method,
null_request_flags);
errorAppendEntry(http->entry, err);
- return -1;
+ ret = -1;
+ goto finish;
}
if ((request = urlParse(method, http->uri)) == NULL) {
debug(33, 5) ("Invalid URL: %s\n", http->uri);
@@ -487,7 +499,8 @@
http->log_type = LOG_TCP_DENIED;
http->entry = clientCreateStoreEntry(http, method,
null_request_flags);
errorAppendEntry(http->entry, err);
- return -1;
+ ret = -1;
+ goto finish;
}
/* compile headers */
/* we should skip request line! */
@@ -500,7 +513,8 @@
http->log_type = LOG_TCP_DENIED;
http->entry = clientCreateStoreEntry(http, method,
null_request_flags);
errorAppendEntry(http->entry, err);
- return -1;
+ ret = -1;
+ goto finish;
}
/*
* If we read past the end of this request, move the remaining
@@ -551,7 +565,8 @@
http->log_type = LOG_TCP_DENIED;
http->entry = clientCreateStoreEntry(http, request->method,
null_request_flags);
errorAppendEntry(http->entry, err);
- return -1;
+ ret = -1;
+ goto finish;
}
if (!clientCheckContentLength(request) || httpHeaderHas(&request->header,
HDR_TRANSFER_ENCODING)) {
err = errorCon(ERR_INVALID_REQ, HTTP_LENGTH_REQUIRED, request);
@@ -559,7 +574,8 @@
http->log_type = LOG_TCP_DENIED;
http->entry = clientCreateStoreEntry(http, request->method,
null_request_flags);
errorAppendEntry(http->entry, err);
- return -1;
+ ret = -1;
+ goto finish;
}
http->request = requestLink(request);
http->orig_request = requestLink(request);
@@ -577,7 +593,8 @@
http->log_type = LOG_TCP_DENIED;
http->entry = clientCreateStoreEntry(http,
urlMethodGetKnownByCode(METHOD_NONE), null_request_flags);
errorAppendEntry(http->entry, err);
- return -1;
+ ret = -1;
+ goto finish;
}
}
if (request->method->code == METHOD_CONNECT) {
@@ -591,7 +608,8 @@
(ObjPackMethod) & httpRequestPackDebug);
debugObj(33, 1, "This request:\n", request, (ObjPackMethod) &
httpRequestPackDebug);
}
- return -2;
+ ret = -2;
+ goto finish;
} else {
clientCheckFollowXForwardedFor(http);
}
@@ -614,12 +632,16 @@
http->log_type = LOG_TCP_DENIED;
http->entry = clientCreateStoreEntry(http, method,
null_request_flags);
errorAppendEntry(http->entry, err);
- return -1;
- }
- return 0;
- }
- if (!cbdataValid(conn))
- return -1;
+ ret = -1;
+ goto finish;
+ }
+ ret = 0;
+ goto finish;
+ }
+ if (!cbdataValid(conn)) {
+ ret = -1;
+ goto finish;
+ }
/*
* For now we assume "here" means "we parsed a valid request. This
might not be the case
@@ -629,7 +651,11 @@
*/
assert(http != NULL);
assert(http->req_sz > 0);
-
- return http->req_sz;
+ ret = http->req_sz;
+
+finish:
+ if (method)
+ urlMethodFree(method);
+ return ret;
}
=======================================
--- /branches/LUSCA_HEAD/src/htcp.c Mon Feb 15 18:41:32 2010
+++ /branches/LUSCA_HEAD/src/htcp.c Mon Apr 5 00:40:47 2010
@@ -578,6 +578,7 @@
method = urlMethodGetKnownByCode(METHOD_GET);
}
s->request = urlParse(method, s->uri);
+ urlMethodFree(method);
return s;
}
--
You received this message because you are subscribed to the Google Groups
"lusca-commit" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/lusca-commit?hl=en.
