Re: [PATCH] BUG/MINOR: http-htx: make sure to print warn on c-l error
Le 13/11/2020 à 13:35, William Dauchy a écrit : Hello Christopher, On Fri, Nov 13, 2020 at 11:14 AM Christopher Faulet wrote: After testing, the 2.1 and 2.0 are not affected because the warning is handled by the main parsing function cfg_parse_listen(). Or I missed something :) indeed I did not test en 2.1 and 2.0 but only on 2.2 relying on the commit I was trying to fix. But on the 2.2, there are 2 cases where the warning is missing. The parsing of the errorfile directive from an http-errors section and the parsing of the http-error directive in a proxy section. Here is a quick patch that should emit a warning in all cases for the 2.2. Could you confirm it fixes the issue you observed ? yes indeed, it is a saner patch here which fixes the two cases I wanted to fix. Thanks! Note also an error is reported on 2.3 and above, thus there is no problem on these versions. yes sure, we only make sure to not change the error path in the future. Ok, I push the patch in upstream and backported it as far as 2.2. It does not fix any real bug in later versions. But if some warnings are emitted at this place in the future, they will be caught. Thanks William, -- Christopher Faulet
Re: [PATCH] BUG/MINOR: http-htx: make sure to print warn on c-l error
Hello Christopher, On Fri, Nov 13, 2020 at 11:14 AM Christopher Faulet wrote: > After testing, the 2.1 and 2.0 are not affected because the warning is handled > by the main parsing function cfg_parse_listen(). Or I missed something :) indeed I did not test en 2.1 and 2.0 but only on 2.2 relying on the commit I was trying to fix. > But on the 2.2, there are 2 cases where the warning is missing. The parsing of > the errorfile directive from an http-errors section and the parsing of the > http-error directive in a proxy section. > > Here is a quick patch that should emit a warning in all cases for the 2.2. > Could > you confirm it fixes the issue you observed ? yes indeed, it is a saner patch here which fixes the two cases I wanted to fix. Thanks! > Note also an error is reported on 2.3 and above, thus there is no problem on > these versions. yes sure, we only make sure to not change the error path in the future. -- William
Re: [PATCH] BUG/MINOR: http-htx: make sure to print warn on c-l error
Le 11/11/2020 à 14:23, William Dauchy a écrit :
commit "BUG/MINOR: http-htx: Just warn if payload of an errorfile
doesn't match the C-L" (which is only present in 2.2, 2.1 and 2.0 trees,
i.e see commit 7bf3d81d3cf4b9f4587 in 2.2 tree), is changing the
behavior of `http_str_to_htx` function as the error path is filling
up errmsg without returning an error.
- when called in `http_htx_init`, it is ok as we test `errmsg` and print
a warning.
- but it is not the case in `http_load_errorfile` and `http_load_errormsg`.
End result is no warning is printed when there is a content-length
mismatch. This patch tries to address that.
This patch is probably only valid for 2.2, 2.1 and 2.0 trees as it fixes
an issue present in those trees. However it might be worth considering
for the dev tree to avoid future regressions and be more consistent.
Signed-off-by: William Dauchy
---
src/http_htx.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/http_htx.c b/src/http_htx.c
index 51dbf44a6..27c409e4a 100644
--- a/src/http_htx.c
+++ b/src/http_htx.c
@@ -1197,6 +1197,9 @@ struct buffer *http_load_errorfile(const char *file, char
**errmsg)
free(http_errmsg);
goto out;
}
+ else if (*errmsg) {
+ ha_warning("invalid custom message %s: %s\n", file, *errmsg);
+ }
/* Insert the node in the tree and return the HTX message */
http_errmsg->msg = chk;
@@ -1248,6 +1251,9 @@ struct buffer *http_load_errormsg(const char *key, const
struct ist msg, char **
free(http_errmsg);
goto out;
}
+ else if (*errmsg) {
+ ha_warning("invalid custom message: %s\n", *errmsg);
+ }
/* Insert the node in the tree and return the HTX message */
http_errmsg->msg = chk;
Hi William,
After testing, the 2.1 and 2.0 are not affected because the warning is handled
by the main parsing function cfg_parse_listen(). Or I missed something :)
But on the 2.2, there are 2 cases where the warning is missing. The parsing of
the errorfile directive from an http-errors section and the parsing of the
http-error directive in a proxy section.
Here is a quick patch that should emit a warning in all cases for the 2.2. Could
you confirm it fixes the issue you observed ?
Note also an error is reported on 2.3 and above, thus there is no problem on
these versions.
--
Christopher Faulet
>From c2afdcbc8f00b14ed8a746e74de750ffa3377ec3 Mon Sep 17 00:00:00 2001
From: Christopher Faulet
Date: Fri, 13 Nov 2020 10:58:01 +0100
Subject: [PATCH] BUG/MINOR: http-htx: make sure to print warn on c-l error
commit "BUG/MINOR: http-htx: Just warn if payload of an errorfile
doesn't match the C-L" (which is only present in 2.2, 2.1 and 2.0 trees,
i.e see commit 7bf3d81d3cf4b9f4587 in 2.2 tree), is changing the
behavior of `http_str_to_htx` function. It may now emit warnings. And,
it is the caller responsibility to display it.
But the warning is missing when an 'http-error' directive is parsed from
a proxy section. It is also missing when an 'errorfile' directive is
parsed from a http-errors section.
This bug only exists on the 2.2. On earlier versions, these directives
are not supported and on later ones, an error is triggered instead of a
warning. Thus, there is no upstream commit ID for this patch and no
backport is needed.
Thanks to William Dauchy that spotted the bug.
---
src/http_htx.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/http_htx.c b/src/http_htx.c
index 51dbf44a6d..162dfc5463 100644
--- a/src/http_htx.c
+++ b/src/http_htx.c
@@ -1993,6 +1993,9 @@ static int proxy_parse_http_error(char **args, int section, struct proxy *curpx,
conf_err->line = line;
LIST_ADDQ(>conf.errors, _err->list);
+ /* handle warning message */
+ if (*errmsg)
+ ret = 1;
out:
return ret;
@@ -2206,6 +2209,10 @@ static int cfg_parse_http_errors(const char *file, int linenum, char **args, int
err_code |= ERR_ALERT | ERR_FATAL;
goto out;
}
+ if (errmsg) {
+ ha_warning("parsing [%s:%d] : %s: %s\n", file, linenum, args[0], errmsg);
+ err_code |= ERR_WARN;
+ }
reply = calloc(1, sizeof(*reply));
if (!reply) {
--
2.26.2
[PATCH] BUG/MINOR: http-htx: make sure to print warn on c-l error
commit "BUG/MINOR: http-htx: Just warn if payload of an errorfile
doesn't match the C-L" (which is only present in 2.2, 2.1 and 2.0 trees,
i.e see commit 7bf3d81d3cf4b9f4587 in 2.2 tree), is changing the
behavior of `http_str_to_htx` function as the error path is filling
up errmsg without returning an error.
- when called in `http_htx_init`, it is ok as we test `errmsg` and print
a warning.
- but it is not the case in `http_load_errorfile` and `http_load_errormsg`.
End result is no warning is printed when there is a content-length
mismatch. This patch tries to address that.
This patch is probably only valid for 2.2, 2.1 and 2.0 trees as it fixes
an issue present in those trees. However it might be worth considering
for the dev tree to avoid future regressions and be more consistent.
Signed-off-by: William Dauchy
---
src/http_htx.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/http_htx.c b/src/http_htx.c
index 51dbf44a6..27c409e4a 100644
--- a/src/http_htx.c
+++ b/src/http_htx.c
@@ -1197,6 +1197,9 @@ struct buffer *http_load_errorfile(const char *file, char
**errmsg)
free(http_errmsg);
goto out;
}
+ else if (*errmsg) {
+ ha_warning("invalid custom message %s: %s\n", file, *errmsg);
+ }
/* Insert the node in the tree and return the HTX message */
http_errmsg->msg = chk;
@@ -1248,6 +1251,9 @@ struct buffer *http_load_errormsg(const char *key, const
struct ist msg, char **
free(http_errmsg);
goto out;
}
+ else if (*errmsg) {
+ ha_warning("invalid custom message: %s\n", *errmsg);
+ }
/* Insert the node in the tree and return the HTX message */
http_errmsg->msg = chk;
--
2.28.0
