Re: appsession fix

From: Willy Tarreau <w#1wt.eu
<mailto:w#1wt.eu?Subject=Re:%20appsession%20fix> > Date: Wed, 13 Feb 2008 11:32:34 +0100

hello,

On Tue, Feb 12, 2008 at 02:21:52PM -0700, Ryan Warnick wrote:
> We've been trying to use the latest release (1.3.14.2) of haproxy to do
> sticky sessions. Cookie insertion is not an option for us, although we
> would much rather use it, as we are trying to work around a problem where
> cookies are unreliable. The appsession functionality only partially worked
> (it wouldn't read the session id out of a query string) until we made the
> following code change to the get_srv_from_appsession function in
> proto_http.c:
>
>
>
> This code
>
> if (appsession_hash_lookup(&(t->be->htbl_proxy),
> asession_temp->sessid) == NULL) {
>
> was changed to
>
> asession_temp = appsession_hash_lookup(&(t->be->htbl_proxy),
> asession_temp->sessid);
> if (asession_temp == NULL) {

Please use the "diff -u" format to submit changes. It's designed exactly for this usage. I will examinate your change, it possibly is good (especially if it fixes the problem for you).

> Also, we think it would be nice to allow for a little more flexibility in
> handling session ids in query strings.

What type of flexibility exactly are you trying to achieve, and to help in what situation ?

> So, we would also like to propose
> the following code change also in the get_srv_from_appsession function in
> proto_http.c (changes in red):
>
>
>
> This code
>
>
>
> 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)))
>
> return;
>
>
>
> /* skip ';' */
>
>
>
> was changed to
>
>
>
> if (t->be->appsession_name == NULL ||
>
> (t->txn.meth != HTTP_METH_GET && t->txn.meth != HTTP_METH_POST)
> ||
>
> (request_line = strstr(begin, t->be->appsession_name)) == NULL
> ||
>
> ((t->be->appsession_name_len + 1 + t->be->appsession_len) >
> (begin + len - request_line)))
>
> return;
>
>
>
> /* skip ';'
>
> request_line++; */
>
>
>
> It is likely that the strstr call will not be as fast as the memchr call,
> but we need this additional flexibility.

It's not only extremely slow, it's completely wrong in two aspects :

• no check on the length of the string, so you're searching for a string containing the name of your cookie in the whole buffer
• the matching can go even beyond the end of the buffer if this one does not contain any zero to stop strstr(). That means that you can match in other buffers which would be allocated after it or even cause a segfault if you don't have any zero between the end of your buffer and the end of the process' VMA.

> Will you be able to make either of these changes part of the standard
> distribution?

Not like this, but if you describe what you *really* need, it's very likely that there is a clean solution not causing regressions or risks for other users. And it is that solution which should be merged.

Regards,
Willy Received on 2008/02/13 11:32 Received on 2008/02/14 21:00