diff --git a/review/pre-2.5-brian.review b/review/pre-2.5-brian.review
index dc9f5802..2ffb1ec9 100644
--- a/review/pre-2.5-brian.review
+++ b/review/pre-2.5-brian.review
@@ -1406,34 +1406,6 @@ static char *msre_action_phase_validate(msre_engine *engine, msre_action *action
item.resolution.label.validNeedsfixing
item.status.label.open
-
-
- 2008-01-07 :: 21:28:41:095 GMT-08:00
- 2008-01-07 :: 21:40:15:511 GMT-08:00
-
- brian
- brian
- apache2/msc_reqbody.c
- item.type.label.programLogic
- item.severity.label.major
- Potential memory leak if modsecurity_request_body_store_disk() fails. Returning here causes modsecurity_request_body_end() to never be called and never free chunk data. See also notes in read_request_body() in apache_io.c.
- /* Write the data we keep in memory */
- chunks = (msc_data_chunk **)msr->msc_reqbody_chunks->elts;
- for(i = 0; i < msr->msc_reqbody_chunks->nelts; i++) {
- disklen += chunks[i]->length;
-
- if (modsecurity_request_body_store_disk(msr, chunks[i]->data, chunks[i]->length, error_msg) < 0) {
- return -1;
- }
-
- free(chunks[i]->data);
- chunks[i]->data = NULL;
- }
-
-
- item.resolution.label.validNeedsfixing
- item.status.label.open
-
2008-01-07 :: 21:42:51:192 GMT-08:00
@@ -3116,5 +3088,349 @@ static char *multipart_construct_filename(modsec_rec *msr) {
item.resolution.label.validNeedsfixing
item.status.label.open
+
+
+ 2008-01-10 :: 12:24:59:317 GMT-08:00
+ 2008-01-10 :: 12:25:40:036 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_multipart.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ len is always assumed to be at least 1. What is preventing a len of 0?
+ if (len > 1) {
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 12:27:34:472 GMT-08:00
+ 2008-01-10 :: 13:16:19:644 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_multipart.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Use MULTIPART_BUF_SIZE for the constant.
+ if (strlen(new_value) > 4096) {
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:04:27:048 GMT-08:00
+ 2008-01-10 :: 13:04:57:229 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_multipart.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Cannot find anything that supports that this is or is not allowed.
+ /* Flag for whitespace after parameter name. */
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:05:06:226 GMT-08:00
+ 2008-01-10 :: 13:05:11:552 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_multipart.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Cannot find anything that supports that this is or is not allowed.
+ /* Flag for whitespace before parameter value. */
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:12:20:299 GMT-08:00
+ 2008-01-10 :: 13:12:45:495 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_multipart.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Use '\r' vs 0x0d as is done elsewhere.
+ if ((c == 0x0d)&&(msr->mpd->bufleft == 1)) {
+ /* we don't want to take 0x0d as the last byte in the buffer */
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:12:54:295 GMT-08:00
+ 2008-01-10 :: 13:13:12:297 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_multipart.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Use '\n' vs 0x0a as is done elsewhere.
+ if ((c == 0x0a)||(msr->mpd->bufleft == 0)||(process_buffer)) {
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:19:16:539 GMT-08:00
+ 2008-01-10 :: 13:20:00:685 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_parsers.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Headers are not used. I think they were added for iconv support, but Ivan removed it.
+ #include "iconv.h"
+#include <errno.h>
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:26:56:609 GMT-08:00
+ 2008-01-10 :: 13:27:21:817 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_util.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Be more consistent in naming.
+ #define VALID_HEX(X) (((X >= '0')&&(X <= '9')) || ((X >= 'a')&&(X <= 'f')) || ((X >= 'A')&&(X <= 'F')))
+#define ISODIGIT(X) ((X >= '0')&&(X <= '7'))
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:32:29:278 GMT-08:00
+ 2008-01-10 :: 13:34:07:812 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_util.c
+ item.type.label.optimization
+ item.severity.label.trivial
+ No need to do a strlen twice *and* loop through the string. Just loop and exit on non-space.
+ if (strlen(string) == 0) return 1;
+
+ for(i = 0; i < strlen(string); i++) {
+ if (!isspace(string[i])) {
+ return 0;
+ }
+ }
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:35:40:307 GMT-08:00
+ 2008-01-10 :: 13:36:03:933 GMT-08:00
+
+ brian
+ brian
+ apache2/modsecurity.c
+ item.type.label.irrelevant
+ item.severity.label.trivial
+ Remove commented code?
+ /* Serial audit log mutext */
+ rc = apr_global_mutex_create(&msce->auditlog_lock, NULL, APR_LOCK_DEFAULT, mp);
+ if (rc != APR_SUCCESS) {
+ //ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, "mod_security: Could not create modsec_auditlog_lock");
+ //return HTTP_INTERNAL_SERVER_ERROR;
+ return -1;
+ }
+
+ #ifdef __SET_MUTEX_PERMS
+ rc = unixd_set_global_mutex_perms(msce->auditlog_lock);
+ if (rc != APR_SUCCESS) {
+ // ap_log_error(APLOG_MARK, APLOG_ERR, rc, s, "mod_security: Could not set permissions on modsec_auditlog_lock; check User and Group directives");
+ // return HTTP_INTERNAL_SERVER_ERROR;
+ return -1;
+ }
+ #endif
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:36:28:229 GMT-08:00
+ 2008-01-10 :: 13:36:52:702 GMT-08:00
+
+ brian
+ brian
+ apache2/modsecurity.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ What *should* we do on error here?
+ if (rc != APR_SUCCESS) {
+ // ap_log_error(APLOG_MARK, APLOG_ERR, rs, s, "Failed to child-init auditlog mutex");
+ }
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:37:17:326 GMT-08:00
+ 2008-01-10 :: 13:37:25:682 GMT-08:00
+
+ brian
+ brian
+ apache2/modsecurity.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ This does nothing.
+ /**
+ * Releases resources held by engine instance.
+ */
+void modsecurity_shutdown(msc_engine *msce) {
+ if (msce == NULL) return;
+}
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:39:01:265 GMT-08:00
+ 2008-01-10 :: 13:39:22:070 GMT-08:00
+
+ brian
+ brian
+ apache2/modsecurity.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Figure out the optimal initial size for the arrays/tables.
+ /* Collections. */
+ msr->tx_vars = apr_table_make(msr->mp, 32);
+ if (msr->tx_vars == NULL) return -1;
+
+ msr->geo_vars = apr_table_make(msr->mp, 8);
+ if (msr->geo_vars == NULL) return -1;
+
+ msr->collections = apr_table_make(msr->mp, 8);
+ if (msr->collections == NULL) return -1;
+ msr->collections_dirty = apr_table_make(msr->mp, 8);
+ if (msr->collections_dirty == NULL) return -1;
+
+ /* Other */
+ msr->tcache = apr_hash_make(msr->mp);
+ if (msr->tcache == NULL) return -1;
+
+ msr->matched_rules = apr_array_make(msr->mp, 16, sizeof(void *));
+ if (msr->matched_rules == NULL) return -1;
+
+ msr->matched_var = (msc_string *)apr_pcalloc(msr->mp, sizeof(msc_string));
+ if (msr->matched_var == NULL) return -1;
+
+ msr->highest_severity = 255; /* high, invalid value */
+
+ msr->removed_rules = apr_array_make(msr->mp, 16, sizeof(char *));
+ if (msr->removed_rules == NULL) return -1;
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:42:33:518 GMT-08:00
+ 2008-01-10 :: 13:42:50:351 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_xml.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Remove #if 0'd code?
+ #if 0
+static void xml_receive_sax_error(void *data, const char *msg, ...) {
+ modsec_rec *msr = (modsec_rec *)data;
+ char message[256];
+
+ if (msr == NULL) return;
+
+ apr_snprintf(message, sizeof(message), "%s (line %d offset %d)",
+ log_escape_nq(msr->mp, msr->xml->parsing_ctx->lastError.message),
+ msr->xml->parsing_ctx->lastError.line,
+ msr->xml->parsing_ctx->lastError.int2);
+
+ msr_log(msr, 5, "XML: Parsing error: %s", message);
+}
+#endif
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:54:16:193 GMT-08:00
+ 2008-01-10 :: 13:54:45:610 GMT-08:00
+
+ brian
+ brian
+ apache2/mod_security2.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ What is the history of this?
+ /* Our own hook to handle RPC transactions (not used at the moment).
+ * // ap_hook_handler(hook_handler, NULL, NULL, APR_HOOK_MIDDLE);
+ */
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+
+
+
+ 2008-01-10 :: 13:56:58:413 GMT-08:00
+ 2008-01-10 :: 13:57:23:209 GMT-08:00
+
+ brian
+ brian
+ apache2/msc_lua.c
+ item.type.label.suggestion
+ item.severity.label.trivial
+ Change C++ to C style comment.
+ // Get the response from the script.
+
+
+ item.resolution.label.validNeedsfixing
+ item.status.label.open
+