From d106a5c4d87cf98c3cda440ab31d0854c05ace24 Mon Sep 17 00:00:00 2001 From: brectanus Date: Thu, 10 Jan 2008 01:06:38 +0000 Subject: [PATCH] Yet more review data. --- review/pre-2.5-brian.review | 212 ++++++++++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) diff --git a/review/pre-2.5-brian.review b/review/pre-2.5-brian.review index ec326e81..dc9f5802 100644 --- a/review/pre-2.5-brian.review +++ b/review/pre-2.5-brian.review @@ -2904,5 +2904,217 @@ static void dummy_free_func(void *data) {} item.resolution.label.validNeedsfixing item.status.label.open + + + 2008-01-09 :: 14:08:14:025 GMT-08:00 + 2008-01-09 :: 14:08:38:129 GMT-08:00 + + brian + brian + apache2/msc_logging.c + item.type.label.suggestion + item.severity.label.trivial + Spelling. + throught + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-09 :: 14:21:32:782 GMT-08:00 + 2008-01-09 :: 14:22:32:803 GMT-08:00 + + brian + brian + apache2/msc_geo.h + item.type.label.suggestion + item.severity.label.trivial + This value may not be portable based on endianness. The algorithm compares it to the IP address as int in host order. + #define GEO_COUNTRY_OFFSET 0xffff00 + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-09 :: 14:31:25:720 GMT-08:00 + 2008-01-09 :: 14:45:20:612 GMT-08:00 + + brian + brian + apache2/msc_geo.c + item.type.label.suggestion + item.severity.label.trivial + Check portability due to endianness. It seems that the DB values are assumed to be in host order. Perhaps the compiled DB is little-endian and we need to compensate? + + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-09 :: 15:28:14:563 GMT-08:00 + 2008-01-09 :: 15:28:24:203 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.irrelevant + item.severity.label.trivial + Remove code? + #if 0 +static char *multipart_construct_filename(modsec_rec *msr) { + char c, *p, *q = msr->mpd->mpp->filename; + char *filename; + + /* find the last backward slash and consider the + * filename to be only what's right from it + */ + p = strrchr(q, '\\'); + if (p != NULL) q = p + 1; + + /* do the same for the forward slash */ + p = strrchr(q, '/'); + if (p != NULL) q = p + 1; + + /* allow letters, digits and dots, replace + * everything else with underscores + */ + p = filename = apr_pstrdup(msr->mp, q); + while((c = *p) != 0) { + if (!( isalnum(c)||(c == '.') )) *p = '_'; + p++; + } + + return filename; +} +#endif + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-09 :: 15:35:50:978 GMT-08:00 + 2008-01-09 :: 15:49:54:128 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.programLogic + item.severity.label.trivial + The multipart C-D header is case insensitive (rfc 2183), so we should probably use strncasecmp() here. + /* accept only what we understand */ + if (strncmp(c_d_value, "form-data", 9) != 0) { + return -1; + } + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-09 :: 16:05:05:601 GMT-08:00 + 2008-01-09 :: 16:07:17:436 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + This allows for a name of "", so maybe check for name[0] == '\0' and return an error. Although we catch this later on as an unrecognized name and return -10. + start = p; + while((*p != '\0')&&(*p != '=')&&(*p != '\t')&&(*p != ' ')) p++; + if (*p == '\0') return -4; + + name = apr_pstrmemdup(msr->mp, start, (p - start)); + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-09 :: 16:14:36:307 GMT-08:00 + 2008-01-09 :: 17:02:47:324 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + For a quoted value we just include everything until the end quote. The field values should be US-ASCII 'qtext' or 'quoted-pair' (RFC 822) or escaped using RFC 2047. + if (*p == '"') { + *t = '\0'; + break; + } + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-09 :: 16:59:12:520 GMT-08:00 + 2008-01-09 :: 17:00:55:566 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + I think this is wrong. RFC 822 defines a quoted-string as <"> *(qtext/quoted-pair) <"> and quoted-par being able to quote any CHAR. + /* only " and \ can be escaped */ + if ((*(p + 1) == '"')||(*(p + 1) == '\\')) { + p++; + } + else { + /* improper escaping */ + + /* We allow for now because IE sends + * improperly escaped content and there's + * nothing we can do about it. + * + * return -9; + */ + } + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-09 :: 17:04:30:357 GMT-08:00 + 2008-01-09 :: 17:05:19:369 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + RFC 2045 defines an 'attribute' as "ALWAYS case-insensitive", so these should be strncasecmp() + if (strcmp(name, "name") == 0) { + if (msr->mpd->mpp->name != NULL) return -14; + msr->mpd->mpp->name = value; + + if (msr->txcfg->debuglog_level >= 9) { + msr_log(msr, 9, "Multipart: Content-Disposition name: %s", + log_escape_nq(msr->mp, value)); + } + } + else + if (strcmp(name, "filename") == 0) { + + + item.resolution.label.validNeedsfixing + item.status.label.open +