On Tue, Apr 06, 2010 at 09:11:10PM +0200, Cyril Bonté wrote:
> Le mardi 6 avril 2010 20:50:50, Willy Tarreau a écrit :
> > > Well, to finalize the patch, what do you prefer ? accept ASPSESSIONIDXXX
> > > (which didn't work) or strictly detect ASPSESSIONID ?
> >
> > I don't want to do important changes in 1.3, so I'd rather have the length
> > correctly checked and ensure that we don't suddenly see the appsession code
> > work differently, or it will scare users away.
> >
> > However for 1.4, I'd prefer that the code does what the config states. That
> > means correct length check while keeping support your prefix patch.
> >
> > I don't know if my explanations are clear, otherwise please ask again :-)
>
> Ok, I think it's clear. Then here are the patches, if they don't represent
> your explanations, please explain again, I'll update :-)
>
> Also, while testing, I noticed that HEAD requests where not available for
> URIs containing the appsession parameter.
Ah, that's amusing. I really think that appsession users use it in very
specific environments or for very specific applications, which explains
why we're still surprized to discover such strange things.
> 1.4.3 patch fixes an horrible segfault I missed in a previous patch when
> appsession is not in the configuration and HAProxy is compiled with
> DEBUG_HASH.
Since this is not used by default, we can merge it at the same time. But
normally, it's preferable that we apply several distinct patches when they
fix different issues. This eases the job for distro maintainers who are
doing backports of some patches they pick in last releases.
Well, then I'm applying your patches. Thanks Cyril!
Willy
> diff -Naur haproxy-1.3.24//src/proto_http.c
> haproxy-1.3.24-appsession//src/proto_http.c
> --- haproxy-1.3.24//src/proto_http.c 2010-03-30 09:56:00.000000000 +0200
> +++ haproxy-1.3.24-appsession//src/proto_http.c 2010-04-06
> 20:36:37.000000000 +0200
> @@ -3720,7 +3720,8 @@
> }
>
> if ((t->be->appsession_name != NULL) &&
> - (memcmp(p1, t->be->appsession_name, p2 -
> p1) == 0)) {
> + (t->be->appsession_name_len == p2 - p1) &&
> + (memcmp(p1, t->be->appsession_name,
> t->be->appsession_name_len) == 0)) {
> /* first, let's see if the cookie is
> our appcookie*/
>
> /* Cool... it's the right one */
> @@ -4138,7 +4139,8 @@
> }
> /* next, let's see if the cookie is our appcookie */
> else if ((t->be->appsession_name != NULL) &&
> - (memcmp(p1, t->be->appsession_name, p2 - p1)
> == 0)) {
> + (t->be->appsession_name_len == p2 - p1) &&
> + (memcmp(p1, t->be->appsession_name,
> t->be->appsession_name_len) == 0)) {
>
> /* Cool... it's the right one */
>
> @@ -4303,7 +4305,7 @@
> char *request_line;
>
> if (t->be->appsession_name == NULL ||
> - (t->txn.meth != HTTP_METH_GET && t->txn.meth != HTTP_METH_POST) ||
> + (t->txn.meth != HTTP_METH_GET && t->txn.meth != HTTP_METH_POST &&
> t->txn.meth != HTTP_METH_HEAD) ||
> (request_line = memchr(begin, ';', len)) == NULL ||
> ((1 + t->be->appsession_name_len + 1 + t->be->appsession_len) >
> (begin + len - request_line)))
> return;
> diff -Naur haproxy-1.3.24//src/sessionhash.c
> haproxy-1.3.24-appsession//src/sessionhash.c
> --- haproxy-1.3.24//src/sessionhash.c 2010-03-30 09:56:00.000000000 +0200
> +++ haproxy-1.3.24-appsession//src/sessionhash.c 2010-04-06
> 20:12:59.000000000 +0200
> @@ -114,6 +114,9 @@
> unsigned int idx;
> appsess *item;
>
> + if (! hash->table)
> + return;
> +
> printf("Dumping hashtable 0x%p\n", hash);
> for (idx = 0; idx < TABLESIZE; idx++) {
> /* we don't even need to call _safe because we return at once */
> diff -Naur haproxy-1.4.3//src/proto_http.c
> haproxy-1.4.3-appsession//src/proto_http.c
> --- haproxy-1.4.3//src/proto_http.c 2010-03-30 09:50:08.000000000 +0200
> +++ haproxy-1.4.3-appsession//src/proto_http.c 2010-04-06
> 20:37:36.000000000 +0200
> @@ -5776,7 +5776,8 @@
> }
>
> /* let's see if the cookie is our
> appcookie */
> - if (memcmp(p1, t->be->appsession_name,
> cmp_len) == 0) {
> + if ((cmp_len ==
> t->be->appsession_name_len) &&
> + (memcmp(p1, t->be->appsession_name,
> t->be->appsession_name_len) == 0)) {
> /* Cool... it's the right one */
>
> manage_client_side_appsession(t, value_begin, value_len);
> }
> @@ -6218,7 +6219,8 @@
> value_len = MIN(t->be->appsession_len,
> p4 - p3);
> }
>
> - if (memcmp(p1, t->be->appsession_name, cmp_len)
> == 0) {
> + if ((cmp_len == t->be->appsession_name_len) &&
> + (memcmp(p1, t->be->appsession_name,
> t->be->appsession_name_len) == 0)) {
> /* Cool... it's the right one */
> if (txn->sessid != NULL) {
> /* free previously allocated
> memory as we don't need it anymore */
> @@ -6279,8 +6281,10 @@
> }
>
> #if defined(DEBUG_HASH)
> - Alert("manage_server_side_cookies\n");
> - appsession_hash_dump(&(t->be->htbl_proxy));
> + if (t->be->appsession_name) {
> + Alert("manage_server_side_cookies\n");
> + appsession_hash_dump(&(t->be->htbl_proxy));
> + }
> #endif
> }
>
> @@ -6386,7 +6390,7 @@
> 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)) {
> + (t->txn.meth != HTTP_METH_GET && t->txn.meth != HTTP_METH_POST &&
> t->txn.meth != HTTP_METH_HEAD)) {
> return;
> }
>
> diff -Naur haproxy-1.4.3//src/sessionhash.c
> haproxy-1.4.3-appsession//src/sessionhash.c
> --- haproxy-1.4.3//src/sessionhash.c 2010-03-30 09:50:08.000000000 +0200
> +++ haproxy-1.4.3-appsession//src/sessionhash.c 2010-04-06
> 08:38:29.000000000 +0200
> @@ -114,6 +114,9 @@
> unsigned int idx;
> appsess *item;
>
> + if (! hash->table)
> + return;
> +
> printf("Dumping hashtable 0x%p\n", hash);
> for (idx = 0; idx < TABLESIZE; idx++) {
> /* we don't even need to call _safe because we return at once */
