Hi Cyril,
On Sun, Nov 29, 2009 at 08:04:48PM +0100, Cyril Bonté wrote:
> Hello,
> considering what we said in this thread, here is the complete patch.
OK. First, let me say I'm really satisfied by the quality of this
patch, I suppose you have spent quite a part of your sunday on it.
I just have a few comments (as usual) below :
> +/* appsession */
> +#define PR_O2_AS_REQL 0x00004000 /* learn the session id from
> the request */
> +#define PR_O2_AS_PFX 0x00008000 /* match on the cookie prefix */
> +#define PR_O2_AS_M_PP 0x00000000 /* path-parameters mode (the
> default mode) */
> +#define PR_O2_AS_M_QS 0x00010000 /* query-string mode */
> +#define PR_O2_AS_M_ANY (PR_O2_AS_M_PP | PR_O2_AS_M_QS)
It's dangerous to set the PR_O2_AS_M_* values together with the other ones
without a comment first and an explicit bit field for the mask later. The
risk is that if one day we add another value, the mask may or may not be
enlarged to support it, and it is possible that someone will believe
the 0x00000000 is an error and will like to fix it.
The suggested way to to that is to enclose the values between the comment
and the mask like here (though any other proposal might fit) :
/* appsession */
#define PR_O2_AS_REQL 0x00004000 /* learn the session id from the
request */
#define PR_O2_AS_PFX 0x00008000 /* match on the cookie prefix */
/* Encoding of appsession cookie matching modes : 2 possible values => 1 bit */
#define PR_O2_AS_M_PP 0x00000000 /* path-parameters mode (the default
mode) */
#define PR_O2_AS_M_QS 0x00010000 /* query-string mode */
#define PR_O2_AS_M_ANY 0x00010000 /* mask covering all PR_O2_AS_M_*
values */
> void get_srv_from_appsession(struct session *t, const char *begin, int len)
> {
> - char *request_line;
> + char *end_params, *first_param, *cur_param, *next_param;
> + char separator;
> + int value_len;
> +
> + int mode = t->be->options2 & PR_O2_AS_M_ANY;
>
> if (t->be->appsession_name == NULL ||
> - (t->txn.meth != HTTP_METH_GET && t->txn.meth != HTTP_METH_POST) ||
> - (request_line = memchr(begin, ';', len)) == NULL ||
> - ((1 + t->be->appsession_name_len + 1 + t->be->appsession_len) >
> (begin + len - request_line)))
> + (t->txn.meth != HTTP_METH_GET && t->txn.meth != HTTP_METH_POST)) {
> return;
> + }
I may be wrong but I think we lost a length check above and that
we'll always onsider we have <len> bytes available in the buffer
so that memchr(begin, ';', len) has no chance of going beyond the
buffer's limit. Oh well, maybe it was already like that :-/
Could you please check on your side and confirm/infirm my doubts ?
Basically I want to ensure we never dereference the buffer past its
end, so begin+len bust always be below the buffer size. If you think
a control is missing, we can merge it as a separate patch because
it's already missing in current code then.
I'm just waiting for your response on this possible issue and I'm
OK to merge it. Please tell me if you'd prefer to resend a different
patch with the cosmetic changes or if I can do them myself. While
you're at it, I noticed a mis-indented "if" statement alone in the
remaining part. Also something I can fix if needed.
Thanks!
Willy
