httpd patterns double free

[Alexander Schrijver]

I ran into this issue when setting up my public_html folder using this configuration.

prefork 2

server "default" {
listen on * port 80

location match "/~*" {
root "/users"
}
}

types {
text/css css
text/html html htm
text/txt txt
image/gif gif
image/jpeg jpeg jpg
image/png png
application/javascript js
application/xml xml
}

The patch below fixed it for me. (server_close_http calls str_match_free and
server_close_http can be called more than once.)

Index: patterns.c
===================================================================
RCS file: /backup/mirrors/cvsync/src/usr.sbin/httpd/patterns.c,v
retrieving revision 1.4
diff -u -p -u -r1.4 patterns.c
--- patterns.c 18 Aug 2015 08:26:39 -0000 1.4
+++ patterns.c 11 Feb 2016 14:45:55 -0000
@@ -708,5 +708,6 @@ str_match_free(struct str_match *m)
for (i = 0; i < m->sm_nmatch; i++)
free(m->sm_match[i]);
free(m->sm_match);
+ m->sm_match = NULL;
m->sm_nmatch = 0;
}

[Martin Natano]

Post by Alexander Schrijver
I ran into this issue when setting up my public_html folder using this configuration.
prefork 2
server "default" {
listen on * port 80
location match "/~*" {
root "/users"
}
}
types {
text/css css
text/html html htm
text/txt txt
image/gif gif
image/jpeg jpeg jpg
image/png png
application/javascript js
application/xml xml
}
The patch below fixed it for me. (server_close_http calls str_match_free and
server_close_http can be called more than once.)

Are you sure the issue you ran into is caused by server_close_http()
being called twice? The only caller thereof is server_free(), which also
frees the client struct. This would result in use after free in
server_close_http() even before the str_match_free() call. I would
rather guess that the sequence in question is server_close_http() being
called after server_reset_http().

However, your patch looks good to me.

natano

Post by Alexander Schrijver
Index: patterns.c
===================================================================
RCS file: /backup/mirrors/cvsync/src/usr.sbin/httpd/patterns.c,v
retrieving revision 1.4
diff -u -p -u -r1.4 patterns.c
--- patterns.c 18 Aug 2015 08:26:39 -0000 1.4
+++ patterns.c 11 Feb 2016 14:45:55 -0000
@@ -708,5 +708,6 @@ str_match_free(struct str_match *m)
for (i = 0; i < m->sm_nmatch; i++)
free(m->sm_match[i]);
free(m->sm_match);
+ m->sm_match = NULL;
m->sm_nmatch = 0;
}

[Alexander Schrijver]

Post by Martin Natano
Are you sure the issue you ran into is caused by server_close_http()
being called twice? The only caller thereof is server_free(), which also
frees the client struct. This would result in use after free in
server_close_http() even before the str_match_free() call. I would
rather guess that the sequence in question is server_close_http() being
called after server_reset_http().
However, your patch looks good to me.
natano

I am sorry, you are right, it was not server_close_http being called twice.
Instead, it was server_reset_http being called twice.

This is the log where I added a printf to server_reset_http and
server_close_http which print the function name and the struct client *clt.

/usr/src/usr.sbin/httpd $ sudo ./httpd -d -vvv
startup
server_privinit: adding server default
socket_rlimit: max open files 1024
socket_rlimit: max open files 1024
server_launch: running server default
server_launch: running server default
server_reset_http 1211d800
server_reset_http 1211d800
default 192.168.1.204 - - [14/Feb/2016:15:47:40 +0100] "GET /~alex/ HTTP/1.1" 200 231
httpd(6456) in free(): error: chunk is already free 0x63512118820
logger exiting, pid 18387
server exiting, pid 5809
parent terminating, pid 4688

[Sebastien Marie]

Post by Alexander Schrijver

Post by Martin Natano
Are you sure the issue you ran into is caused by server_close_http()
being called twice? The only caller thereof is server_free(), which also
frees the client struct. This would result in use after free in
server_close_http() even before the str_match_free() call. I would
rather guess that the sequence in question is server_close_http() being
called after server_reset_http().
However, your patch looks good to me.
natano

I am sorry, you are right, it was not server_close_http being called twice.
Instead, it was server_reset_http being called twice.

The patch makes sens to me also. I am OK with it.

Any other dev in order to commit it ?
--
Sebastien Marie


Index: patterns.c
===================================================================
RCS file: /backup/mirrors/cvsync/src/usr.sbin/httpd/patterns.c,v
retrieving revision 1.4
diff -u -p -u -r1.4 patterns.c
--- patterns.c 18 Aug 2015 08:26:39 -0000 1.4
+++ patterns.c 11 Feb 2016 14:45:55 -0000
@@ -708,5 +708,6 @@ str_match_free(struct str_match *m)
for (i = 0; i < m->sm_nmatch; i++)
free(m->sm_match[i]);
free(m->sm_match);
+ m->sm_match = NULL;
m->sm_nmatch = 0;
}

[Sebastien Marie]

Post by Alexander Schrijver
The patch below fixed it for me. (server_close_http calls str_match_free and
server_close_http can be called more than once.)
Index: patterns.c
===================================================================
RCS file: /backup/mirrors/cvsync/src/usr.sbin/httpd/patterns.c,v
retrieving revision 1.4
diff -u -p -u -r1.4 patterns.c
--- patterns.c 18 Aug 2015 08:26:39 -0000 1.4
+++ patterns.c 11 Feb 2016 14:45:55 -0000
@@ -708,5 +708,6 @@ str_match_free(struct str_match *m)
for (i = 0; i < m->sm_nmatch; i++)
free(m->sm_match[i]);
free(m->sm_match);
+ m->sm_match = NULL;
m->sm_nmatch = 0;
}

Committed, thanks !