[OGo-Developer] Bugs in mod_ngobjweb
Wolfgang Sourdeau
developer@opengroupware.org
Thu, 27 Mar 2008 12:08:14 -0400
This is a multi-part message in MIME format.
--------------090400040508000003050009
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit
Helge Hess a écrit :
> On 26.03.2008, at 18:18, Stéphane Corthésy wrote:
>> I found bugs in mod_ngobjweb:
>>
>> In NGBufferedDescriptor.c, at the beginning of
>> NGBufferedDescriptor_read(), availBytes is initialized by
>> numberOfAvailableReadBufferBytes(self), whereas the test "if(self ==
>> NULL)" is done later; this could crash. BTW,
>> numberOfAvailableReadBufferBytes(self) should be called only just
>> before "if (availBytes >= _len)".
>>
>> In handler.c:
>> Around line 721, after NGBufferedDescriptor_safeRead(), you free
>> 'toApp', but don't NULLify it, and some lines later the pointer is
>> freed again when not NULL.
>> BTW, there is a "#if HEAVY_LOG" whereas everywhere else you use
>> "if(HEAVY_LOG)".
There are still a few lines where setting toApp to NULL is required.
Here is a patch that applies to your latest changes in handler.c.
Along with the NULLification, a cleanup has been made to avoid
duplication of code. And the closing of the connection has been moved to
the end of the function because it should happen unconditionnally to
avoid leaking connections.
You may want to change my "if (var)" to your "if (var != NULL)", but the
rest of the logic has been in production here for many months already
without any problem.
Wolfgang
Index: sope-appserver/mod_ngobjweb/handler.c
===================================================================
--- sope-appserver/mod_ngobjweb/handler.c (révision 1618)
+++ sope-appserver/mod_ngobjweb/handler.c (copie de travail)
@@ -267,7 +267,7 @@
const char *uri;
unsigned requestContentLength;
void *requestBody;
-
+
uri = r->uri;
requestContentLength = 0;
requestBody = NULL;
@@ -404,6 +404,9 @@
"could not create socket in domain %i.", domain);
return DECLINED;
}
+/* else */
+/* ap_log_error(__FILE__, __LINE__, APLOG_ERR, 0, r->server, */
+/* "appFd socket created in domain %i: %d", domain,
appFd); */
if ((result = _connectInstance(r, appFd, address, addressLen)) < 0)
return 500;
@@ -646,7 +649,10 @@
writeErrorHandler:
if (writeError == 1) {
- if (toApp) NGBufferedDescriptor_free(toApp);
+ if (toApp) {
+ NGBufferedDescriptor_free(toApp);
+ toApp = NULL;
+ }
ap_log_error(__FILE__, __LINE__, APLOG_ERR, 0, r->server,
"socket write error during transfer of HTTP header
section");
@@ -659,7 +665,10 @@
if (!NGBufferedDescriptor_safeWrite(toApp,
requestBody,
requestContentLength)) {
- if (toApp) NGBufferedDescriptor_free(toApp);
+ if (toApp) {
+ NGBufferedDescriptor_free(toApp);
+ toApp = NULL;
+ }
ap_log_error(__FILE__, __LINE__, APLOG_ERR, 0, r->server,
"couldn't transfer HTTP req body to app server (%i
bytes)",
contentLength);
@@ -677,7 +686,10 @@
/* read response line */
if (!NGScanResponseLine(toApp, NULL, &statusCode, NULL)) {
- if (toApp) NGBufferedDescriptor_free(toApp);
+ if (toApp) {
+ NGBufferedDescriptor_free(toApp);
+ toApp = NULL;
+ }
ap_log_error(__FILE__, __LINE__, APLOG_ERR, 0, r->server,
"error during reading of response line ..");
return 500;
@@ -716,16 +728,8 @@
}
// read whole response
- if (!NGBufferedDescriptor_safeRead(toApp, buffer, contentLength)) {
- if (toApp) NGBufferedDescriptor_free(toApp);
- }
+ NGBufferedDescriptor_safeRead(toApp, buffer, contentLength);
- // close connection to app
- if (toApp) {
- NGBufferedDescriptor_free(toApp);
- toApp = NULL;
- }
-
ap_log_error(__FILE__, __LINE__, APLOG_INFO, 0, r->server,
"send response (size=%i)",
contentLength);
@@ -739,15 +743,14 @@
int result = 0;
int writeCount = 0;
- do {
- result = NGBufferedDescriptor_read(toApp, buffer, sizeof(buffer));
- if (result > 0) {
- ap_rwrite(buffer, result, r);
- ap_rflush(r);
- writeCount += result;
- }
+ while ((result = NGBufferedDescriptor_read(toApp,
+ buffer,
+ sizeof(buffer))
+ > 0)) {
+ ap_rwrite(buffer, result, r);
+ ap_rflush(r);
+ writeCount += result;
}
- while (result > 0);
if (HEAVY_LOG && (writeCount > 0)) {
ap_log_error(__FILE__, __LINE__, APLOG_INFO, 0, r->server,
@@ -756,10 +759,26 @@
}
}
}
-
+
+ // close connection to app
+ if (toApp) {
+ NGBufferedDescriptor_free(toApp);
+ toApp = NULL;
+ }
+
return OK;
}
+/* int ngobjweb_handler(request_rec *r) { */
+/* int code; */
+
+/* fprintf (stderr, "ngobjweb_handler...\n======================"); */
+/* code = real_ngobjweb_handler(r); */
+/* fprintf (stderr, "================ %d\n", code); */
+
+/* return code; */
+/* } */
+
#if WITH_LOGGING
#if 0
static void test(void) {
--------------090400040508000003050009
Content-Type: text/x-diff;
name="mod_ngobjweb.fixes.diff"
Content-Transfer-Encoding: 8bit
Content-Disposition: inline;
filename="mod_ngobjweb.fixes.diff"
Index: handler.c
===================================================================
--- handler.c (révision 1619)
+++ handler.c (copie de travail)
@@ -646,7 +646,10 @@
writeErrorHandler:
if (writeError == 1) {
- if (toApp) NGBufferedDescriptor_free(toApp);
+ if (toApp) {
+ NGBufferedDescriptor_free(toApp);
+ toApp = NULL;
+ }
ap_log_error(__FILE__, __LINE__, APLOG_ERR, 0, r->server,
"socket write error during transfer of HTTP header section");
@@ -659,7 +662,10 @@
if (!NGBufferedDescriptor_safeWrite(toApp,
requestBody,
requestContentLength)) {
- if (toApp) NGBufferedDescriptor_free(toApp);
+ if (toApp) {
+ NGBufferedDescriptor_free(toApp);
+ toApp = NULL;
+ }
ap_log_error(__FILE__, __LINE__, APLOG_ERR, 0, r->server,
"couldn't transfer HTTP req body to app server (%i bytes)",
contentLength);
@@ -677,7 +683,10 @@
/* read response line */
if (!NGScanResponseLine(toApp, NULL, &statusCode, NULL)) {
- if (toApp) NGBufferedDescriptor_free(toApp);
+ if (toApp) {
+ NGBufferedDescriptor_free(toApp);
+ toApp = NULL;
+ }
ap_log_error(__FILE__, __LINE__, APLOG_ERR, 0, r->server,
"error during reading of response line ..");
return 500;
@@ -716,13 +725,8 @@
}
// read whole response
- if (!NGBufferedDescriptor_safeRead(toApp, buffer, contentLength)) {
- if (toApp != NULL) { NGBufferedDescriptor_free(toApp); toApp = NULL; }
- }
+ NGBufferedDescriptor_safeRead(toApp, buffer, contentLength);
- // close connection to app
- if (toApp != NULL) { NGBufferedDescriptor_free(toApp); toApp = NULL; }
-
ap_log_error(__FILE__, __LINE__, APLOG_INFO, 0, r->server,
"send response (size=%i)",
contentLength);
@@ -736,15 +740,14 @@
int result = 0;
int writeCount = 0;
- do {
- result = NGBufferedDescriptor_read(toApp, buffer, sizeof(buffer));
- if (result > 0) {
- ap_rwrite(buffer, result, r);
- ap_rflush(r);
- writeCount += result;
- }
+ while ((result = NGBufferedDescriptor_read(toApp,
+ buffer,
+ sizeof(buffer))
+ > 0)) {
+ ap_rwrite(buffer, result, r);
+ ap_rflush(r);
+ writeCount += result;
}
- while (result > 0);
if (HEAVY_LOG && (writeCount > 0)) {
ap_log_error(__FILE__, __LINE__, APLOG_INFO, 0, r->server,
@@ -753,7 +756,13 @@
}
}
}
-
+
+ // close connection to app
+ if (toApp) {
+ NGBufferedDescriptor_free(toApp);
+ toApp = NULL;
+ }
+
return OK;
}
--------------090400040508000003050009--