You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2021/11/12 18:12:58 UTC
svn commit: r1894982 - /httpd/apreq/trunk/library/util.c
Author: ylavic
Date: Fri Nov 12 18:12:58 2021
New Revision: 1894982
URL: http://svn.apache.org/viewvc?rev=1894982&view=rev
Log:
apreq_header_attribute: Search for the exact attribute name.
Improve the parsing of the header attributes such that we don't match any
special character before that attribute name (e.g. "(boundary=") or let
forbidden characters unnoticed.
Modified:
httpd/apreq/trunk/library/util.c
Modified: httpd/apreq/trunk/library/util.c
URL: http://svn.apache.org/viewvc/httpd/apreq/trunk/library/util.c?rev=1894982&r1=1894981&r2=1894982&view=diff
==============================================================================
--- httpd/apreq/trunk/library/util.c (original)
+++ httpd/apreq/trunk/library/util.c Fri Nov 12 18:12:58 2021
@@ -838,100 +838,157 @@ APREQ_DECLARE(apr_status_t) apreq_file_m
}
-/*
- * is_2616_token() is the verbatim definition from section 2.2
- * in the rfc itself. We try to optimize it around the
- * expectation that the argument is not a token, which
- * should be the typical usage.
- */
-
-static APR_INLINE
-unsigned is_2616_token(const char c) {
- switch (c) {
- case ' ': case ';': case ',': case '"': case '\t':
- /* The chars we are expecting are listed above;
- the chars below are just for completeness. */
- case '?': case '=': case '@': case ':': case '\\': case '/':
- case '(': case ')':
- case '<': case '>':
- case '{': case '}':
- case '[': case ']':
- return 0;
- default:
- if (apr_iscntrl(c))
- return 0;
- }
- return 1;
-}
+#define IS_SPACE_CHAR(c) ((c) == '\t' || (c) == ' ')
+#define IS_TOKEN_CHAR(c) (apr_isalnum(c) \
+ || ((c) && strchr("!#$%&'*+-.^_`|~", (c))))
APREQ_DECLARE(apr_status_t)
apreq_header_attribute(const char *hdr,
const char *name, const apr_size_t nlen,
const char **val, apr_size_t *vlen)
{
- const char *key, *v;
-
- /* Must ensure first char isn't '=', so we can safely backstep. */
- while (*hdr == '=')
- ++hdr;
+ int done = 0;
- while ((key = strchr(hdr, '=')) != NULL) {
+ if (!nlen)
+ return APREQ_ERROR_NOATTR;
- v = key + 1;
- --key;
-
- while (apr_isspace(*key) && key > hdr + nlen)
- --key;
+ do {
+ const char *hde, *v;
+ apr_size_t tail = 0;
+
+ /* Parse the name => [hdr:hde[ */
+ hde = hdr;
+ look_for_end_name:
+ switch (*hde) {
+ case 0:
+ case '\r':
+ case '\n':
+ done = 1;
+ case '=':
+ case ';':
+ case ',':
+ v = hde + 1;
+ hde -= tail;
+ break;
+ case ' ':
+ case '\t':
+ if (hde == hdr)
+ ++hdr;
+ else
+ ++tail;
+ ++hde;
+ goto look_for_end_name;
+ default:
+ /* The name is a token */
+ if (!IS_TOKEN_CHAR(*hde))
+ return APREQ_ERROR_BADCHAR;
+ /* Nothing after the tail */
+ if (tail)
+ return APREQ_ERROR_BADATTR;
+ ++hde;
+ goto look_for_end_name;
+ }
- key -= nlen - 1;
+ /* Parse the value => (*val, *vlen) */
+ if (v[-1] == '=') {
+ if (hde == hdr) {
+ /* The name can't be empty */
+ return APREQ_ERROR_BADATTR;
+ }
- while (apr_isspace(*v))
- ++v;
+ while (IS_SPACE_CHAR(*v))
+ ++v;
- if (*v == '"') {
- ++v;
- *val = v;
+ /* Quoted string ? */
+ if (*v == '"') {
+ *val = ++v;
+
+ /* XXX: the interface does not permit unescaping,
+ * it should have pool to allocate from.
+ * The caller can't know whether a returned '\\' is
+ * a quoted-char or not..
+ */
+ look_for_end_quote:
+ switch (*v) {
+ case 0:
+ case '\r':
+ case '\n':
+ return APREQ_ERROR_BADSEQ;
+ case '"':
+ *vlen = v - *val;
+ break;
+ case '\\':
+ if (v[1] != 0)
+ ++v;
+ ++v;
+ goto look_for_end_quote;
+ default:
+ if (apr_iscntrl(*v))
+ return APREQ_ERROR_BADCHAR;
+ ++v;
+ goto look_for_end_quote;
+ }
- look_for_end_quote:
- switch (*v) {
- case '"':
- break;
- case 0:
- return APREQ_ERROR_BADSEQ;
- case '\\':
- if (v[1] != 0)
+ look_for_after_quote:
+ switch (*v) {
+ case 0:
+ case '\r':
+ case '\n':
+ done = 1;
+ case ';':
+ case ',':
+ break;
+ case ' ':
+ case '\t':
+ goto look_for_after_quote;
+ default:
+ if (apr_iscntrl(*v))
+ return APREQ_ERROR_BADCHAR;
+ return APREQ_ERROR_BADSEQ;
+ }
+ }
+ else {
+ *val = v;
+ tail = 0;
+
+ look_for_end_value:
+ switch (*v) {
+ case 0:
+ case '\r':
+ case '\n':
+ done = 1;
+ case ';':
+ case ',':
+ *vlen = v - *val - tail;
+ break;
+ case ' ':
+ case '\t':
+ if (*val == v)
+ ++*val;
+ else
+ ++tail;
++v;
- default:
- ++v;
- goto look_for_end_quote;
+ goto look_for_end_value;
+ default:
+ if (apr_iscntrl(*v))
+ return APREQ_ERROR_BADCHAR;
+ ++v;
+ tail = 0;
+ goto look_for_end_value;
+ }
}
}
else {
- *val = v;
-
- look_for_terminator:
- switch (*v) {
- case 0:
- case ' ':
- case ';':
- case ',':
- case '\t':
- case '\r':
- case '\n':
- break;
- default:
- ++v;
- goto look_for_terminator;
- }
+ *val = NULL;
+ *vlen = 0;
}
- if (key >= hdr && strncasecmp(key, name, nlen) == 0) {
- *vlen = v - *val;
- if (key == hdr || ! is_2616_token(key[-1]))
- return APR_SUCCESS;
+ if (hdr + nlen == hde && strncasecmp(hdr, name, nlen) == 0) {
+ return APR_SUCCESS;
}
- hdr = v;
- }
+
+ hdr = v + 1;
+ } while (!done);
return APREQ_ERROR_NOATTR;
}
Re: svn commit: r1894982 - /httpd/apreq/trunk/library/util.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Aug 17, 2022 at 06:17:23PM +0200, Yann Ylavic wrote:
> I fixed it in r1903496 by requiring that the name in a name=value pair
> only be a token, with no equal sign the attribute is a value only.
Thanks a lot for committing all the fixes, test suite is passing here
now.
Regards, Joe
Re: svn commit: r1894982 - /httpd/apreq/trunk/library/util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Aug 17, 2022 at 3:40 PM Joe Orton <jo...@redhat.com> wrote:
>
> On Wed, Aug 17, 2022 at 02:05:09PM +0100, Joe Orton wrote:
> > On Fri, Nov 12, 2021 at 06:12:58PM -0000, ylavic@apache.org wrote:
> > > Author: ylavic
> > > Date: Fri Nov 12 18:12:58 2021
> > > New Revision: 1894982
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1894982&view=rev
> > > Log:
> > > apreq_header_attribute: Search for the exact attribute name.
> > >
> > > Improve the parsing of the header attributes such that we don't match any
> > > special character before that attribute name (e.g. "(boundary=") or let
> > > forbidden characters unnoticed.
>
> Yann, it appears this change is also breaking the "params" test case in
> the apreq test suite. A test is trying to parse a content-type like
> header:
>
> https://svn.apache.org/viewvc/httpd/apreq/trunk/library/t/params.c?revision=1903492&view=markup#l100
>
> it fails when reaching the '/' in "text/plain" which is a non-token
> character:
>
> default:
> /* The name is a token */
> if (!IS_TOKEN_CHAR(*hde))
> return APREQ_ERROR_BADCHAR;
>
> Unless this is an invalid use case (the test case implies otherwise)
> this seems like a regression as well?
I fixed it in r1903496 by requiring that the name in a name=value pair
only be a token, with no equal sign the attribute is a value only.
Regards;
Yann.
Re: svn commit: r1894982 - /httpd/apreq/trunk/library/util.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Aug 17, 2022 at 02:05:09PM +0100, Joe Orton wrote:
> On Fri, Nov 12, 2021 at 06:12:58PM -0000, ylavic@apache.org wrote:
> > Author: ylavic
> > Date: Fri Nov 12 18:12:58 2021
> > New Revision: 1894982
> >
> > URL: http://svn.apache.org/viewvc?rev=1894982&view=rev
> > Log:
> > apreq_header_attribute: Search for the exact attribute name.
> >
> > Improve the parsing of the header attributes such that we don't match any
> > special character before that attribute name (e.g. "(boundary=") or let
> > forbidden characters unnoticed.
Yann, it appears this change is also breaking the "params" test case in
the apreq test suite. A test is trying to parse a content-type like
header:
https://svn.apache.org/viewvc/httpd/apreq/trunk/library/t/params.c?revision=1903492&view=markup#l100
it fails when reaching the '/' in "text/plain" which is a non-token
character:
default:
/* The name is a token */
if (!IS_TOKEN_CHAR(*hde))
return APREQ_ERROR_BADCHAR;
Unless this is an invalid use case (the test case implies otherwise)
this seems like a regression as well?
Regrads, Joe
Re: svn commit: r1894982 - /httpd/apreq/trunk/library/util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Aug 17, 2022 at 3:05 PM Joe Orton <jo...@redhat.com> wrote:
>
> This is an infinite loop. The libapreq test suite is spinning here,
> "make test" from apreq trunk.
Indeed, should be fixed in r1903495.
Thanks;
Yann.
Re: svn commit: r1894982 - /httpd/apreq/trunk/library/util.c
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Nov 12, 2021 at 06:12:58PM -0000, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Nov 12 18:12:58 2021
> New Revision: 1894982
>
> URL: http://svn.apache.org/viewvc?rev=1894982&view=rev
> Log:
> apreq_header_attribute: Search for the exact attribute name.
>
> Improve the parsing of the header attributes such that we don't match any
> special character before that attribute name (e.g. "(boundary=") or let
> forbidden characters unnoticed.
...
> + look_for_after_quote:
> + switch (*v) {
> + case 0:
> + case '\r':
> + case '\n':
> + done = 1;
> + case ';':
> + case ',':
> + break;
> + case ' ':
> + case '\t':
> + goto look_for_after_quote;
This is an infinite loop. The libapreq test suite is spinning here,
"make test" from apreq trunk.