> Please avoid defining macros for such control blocks, especially when
> involve implicit local variables and can cause returns, it's always
> harder to follow and when checking for unrelated bugs, they tend to
> be disturbing. I know we already do it with CHECK_HTTP_MESSAGE_FIRST(),
> but it's used a lot more, something like 25 times or so. And even then
> it's annoying when you have to single-step through it.
>
> Another point is that nobody will ever see this alert, so in practice better
> not even send it and just ensure that the function fails so that the rules
> processing is aborted and an error is returned. I guess -1 already grants
> this given that you added the control in your patch. So that could simply
> result in adding this at the four places where you used CHECK_ACT_REPLACE :
>
> if (trash.len < 0)
> return -1;
> Another point concerning the code's efficiency, for the source I believe
> you should stick to the *str test instead of str < str_end for 3 reasons :
>
> - everywhere it's tested, the following instruction is a fetch of *str,
> so the same register will be reused ;
>
> - on machines with low number of registers (eg: x86), it will avoid
> moving variables back and forth to the stack.
>
> - it avoids having to run over the whole input string twice (once for
> strlen() and a second time for the actual work).
>
Willy:
I am attaching a patch that incorporates your revisions.
--
Sasha Pachev
Fast Running Blog.
http://fastrunningblog.com
Run. Blog. Improve. Repeat.
diff --git a/include/common/regex.h b/include/common/regex.h
index 9789ec3..63689af 100644
--- a/include/common/regex.h
+++ b/include/common/regex.h
@@ -79,7 +79,7 @@ extern regmatch_t pmatch[MAX_MATCH];
* The function return 1 is succes case, else return 0 and err is filled.
*/
int regex_comp(const char *str, struct my_regex *regex, int cs, int cap, char **err);
-int exp_replace(char *dst, char *src, const char *str, const regmatch_t *matches);
+int exp_replace(char *dst, uint dst_size, char *src, const char *str, const regmatch_t *matches);
const char *check_replace_string(const char *str);
const char *chain_regex(struct hdr_exp **head, const regex_t *preg,
int action, const char *replace, void *cond);
diff --git a/src/proto_http.c b/src/proto_http.c
index bd6d024..88e27e2 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -6817,7 +6817,11 @@ int apply_filter_to_req_headers(struct session *s, struct channel *req, struct h
break;
case ACT_REPLACE:
- trash.len = exp_replace(trash.str, cur_ptr, exp->replace, pmatch);
+ trash.len = exp_replace(trash.str, trash.size, cur_ptr, exp->replace, pmatch);
+
+ if (trash.len < 0)
+ return -1;
+
delta = buffer_replace2(req->buf, cur_ptr, cur_end, trash.str, trash.len);
/* FIXME: if the user adds a newline in the replacement, the
* index will not be recalculated for now, and the new line
@@ -6927,7 +6931,11 @@ int apply_filter_to_req_line(struct session *s, struct channel *req, struct hdr_
case ACT_REPLACE:
*cur_end = term; /* restore the string terminator */
- trash.len = exp_replace(trash.str, cur_ptr, exp->replace, pmatch);
+ trash.len = exp_replace(trash.str, trash.size, cur_ptr, exp->replace, pmatch);
+
+ if (trash.len < 0)
+ return -1;
+
delta = buffer_replace2(req->buf, cur_ptr, cur_end, trash.str, trash.len);
/* FIXME: if the user adds a newline in the replacement, the
* index will not be recalculated for now, and the new line
@@ -7676,7 +7684,11 @@ int apply_filter_to_resp_headers(struct session *s, struct channel *rtr, struct
break;
case ACT_REPLACE:
- trash.len = exp_replace(trash.str, cur_ptr, exp->replace, pmatch);
+ trash.len = exp_replace(trash.str, trash.size, cur_ptr, exp->replace, pmatch);
+
+ if (trash.len < 0)
+ return -1;
+
delta = buffer_replace2(rtr->buf, cur_ptr, cur_end, trash.str, trash.len);
/* FIXME: if the user adds a newline in the replacement, the
* index will not be recalculated for now, and the new line
@@ -7766,7 +7778,11 @@ int apply_filter_to_sts_line(struct session *s, struct channel *rtr, struct hdr_
case ACT_REPLACE:
*cur_end = term; /* restore the string terminator */
- trash.len = exp_replace(trash.str, cur_ptr, exp->replace, pmatch);
+ trash.len = exp_replace(trash.str, trash.size, cur_ptr, exp->replace, pmatch);
+
+ if (trash.len < 0)
+ return -1;
+
delta = buffer_replace2(rtr->buf, cur_ptr, cur_end, trash.str, trash.len);
/* FIXME: if the user adds a newline in the replacement, the
* index will not be recalculated for now, and the new line
@@ -7847,7 +7863,8 @@ int apply_filters_to_response(struct session *s, struct channel *rtr, struct pro
/* The filter did not match the response, it can be
* iterated through all headers.
*/
- apply_filter_to_resp_headers(s, rtr, exp);
+ if (unlikely(apply_filter_to_resp_headers(s, rtr, exp) < 0))
+ return -1;
}
}
return 0;
diff --git a/src/regex.c b/src/regex.c
index 7a76940..58ddd30 100644
--- a/src/regex.c
+++ b/src/regex.c
@@ -22,14 +22,18 @@
/* regex trash buffer used by various regex tests */
regmatch_t pmatch[MAX_MATCH]; /* rm_so, rm_eo for regular expressions */
-
-int exp_replace(char *dst, char *src, const char *str, const regmatch_t *matches)
+int exp_replace(char *dst, uint dst_size, char *src, const char *str, const regmatch_t *matches)
{
char *old_dst = dst;
-
+ char* dst_end = dst + dst_size;
+
while (*str) {
if (*str == '\\') {
str++;
+
+ if (!*str)
+ return -1;
+
if (isdigit((unsigned char)*str)) {
int len, num;
@@ -38,6 +42,10 @@ int exp_replace(char *dst, char *src, const char *str, const regmatch_t *matches
if (matches[num].rm_eo > -1 && matches[num].rm_so > -1) {
len = matches[num].rm_eo - matches[num].rm_so;
+
+ if (dst + len >= dst_end)
+ return -1;
+
memcpy(dst, src + matches[num].rm_so, len);
dst += len;
}
@@ -46,19 +54,40 @@ int exp_replace(char *dst, char *src, const char *str, const regmatch_t *matches
unsigned char hex1, hex2;
str++;
+ if (!*str)
+ return -1;
+
hex1 = toupper(*str++) - '0';
+
+ if (!*str)
+ return -1;
+
hex2 = toupper(*str++) - '0';
if (hex1 > 9) hex1 -= 'A' - '9' - 1;
if (hex2 > 9) hex2 -= 'A' - '9' - 1;
+
+ if (dst >= dst_end)
+ return -1;
+
*dst++ = (hex1<<4) + hex2;
} else {
+ if (dst >= dst_end)
+ return -1;
+
*dst++ = *str++;
}
} else {
+ if (dst >= dst_end)
+ return -1;
+
*dst++ = *str++;
}
}
+
+ if (dst >= dst_end)
+ return -1;
+
*dst = '\0';
return dst - old_dst;
}
