Replace gethostbyname() with thread-safe getaddrinfo() #104

2020-02-16T03:20:09+01:00

sth commented

2020-02-16 03:20:09 +01:00

(Migrated from github.com)

get_client_hostname() uses gethostbyname(), which is not thread safe. This is a problem because get_client_hostname() gets called from different threads, for example from the OAuth2 module.

It looks like this is the reason for the segfault in #101.

This is the stack trace that pointed to the problem:

...
#5  <signal handler called>
#6  a_crash () at ./arch/x86_64/atomic_arch.h:108
#7  unmap_chunk (self=<optimized out>) at src/malloc/malloc.c:515
#8  free (p=<optimized out>) at src/malloc/malloc.c:526
#9  0x00007fb328e4ec00 in gethostbyname2 (name=0x7fb328eabe80 <buf> "172.17.0.1", af=2) at src/network/gethostbyname2.c:15
#10 0x00007fb32819d923 in get_client_hostname (request=0x55ad40c3aa60) at /opt/glewlwyd/src/misc.c:98
#11 0x00007fb3281a6684 in check_auth_type_client_credentials_grant (request=0x55ad40c3aa60, response=0x55ad40c3b9c0, user_data=0x55ad40c18c00)
    at /opt/glewlwyd/src/plugin/protocol_oauth2.c:1635
#12 0x00007fb3281a7edd in callback_oauth2_token (request=0x55ad40c3aa60, response=0x55ad40c3b9c0, user_data=0x55ad40c18c00)
    at /opt/glewlwyd/src/plugin/protocol_oauth2.c:1960
#13 0x00007fb328c1ff7f in ulfius_webservice_dispatcher (cls=0x55ad40bb8360, connection=0x55ad40c31a20, url=0x55ad40c32a45 "/api/oauth2/token", 
    method=0x55ad40c32a40 "POST", version=0x55ad40c32a57 "HTTP/1.1", upload_data=0x0, upload_data_size=0x7fb328143440, con_cls=0x55ad40c31a78)
    at /opt/glewlwyd/build/ulfius-src/src/ulfius.c:539
#14 0x00007fb3288a6c23 in ?? () from /usr/lib/libmicrohttpd.so.12
#15 0x00007fb3288a8a97 in ?? () from /usr/lib/libmicrohttpd.so.12
#16 0x00007fb3288a9b25 in ?? () from /usr/lib/libmicrohttpd.so.12
#17 0x00007fb3288aca12 in ?? () from /usr/lib/libmicrohttpd.so.12
#18 0x00007fb328e6933e in start (p=0x7fb3281438c0) at src/thread/pthread_create.c:192
#19 0x00007fb328e6b447 in __clone () at src/thread/x86_64/clone.s:22

`get_client_hostname()` uses `gethostbyname()`, which is not thread safe. This is a problem because `get_client_hostname()` gets called from different threads, for example from the OAuth2 module. It looks like this is the reason for the segfault in #101. This is the stack trace that pointed to the problem: ``` ... #5 <signal handler called> #6 a_crash () at ./arch/x86_64/atomic_arch.h:108 #7 unmap_chunk (self=<optimized out>) at src/malloc/malloc.c:515 #8 free (p=<optimized out>) at src/malloc/malloc.c:526 #9 0x00007fb328e4ec00 in gethostbyname2 (name=0x7fb328eabe80 <buf> "172.17.0.1", af=2) at src/network/gethostbyname2.c:15 #10 0x00007fb32819d923 in get_client_hostname (request=0x55ad40c3aa60) at /opt/glewlwyd/src/misc.c:98 #11 0x00007fb3281a6684 in check_auth_type_client_credentials_grant (request=0x55ad40c3aa60, response=0x55ad40c3b9c0, user_data=0x55ad40c18c00) at /opt/glewlwyd/src/plugin/protocol_oauth2.c:1635 #12 0x00007fb3281a7edd in callback_oauth2_token (request=0x55ad40c3aa60, response=0x55ad40c3b9c0, user_data=0x55ad40c18c00) at /opt/glewlwyd/src/plugin/protocol_oauth2.c:1960 #13 0x00007fb328c1ff7f in ulfius_webservice_dispatcher (cls=0x55ad40bb8360, connection=0x55ad40c31a20, url=0x55ad40c32a45 "/api/oauth2/token", method=0x55ad40c32a40 "POST", version=0x55ad40c32a57 "HTTP/1.1", upload_data=0x0, upload_data_size=0x7fb328143440, con_cls=0x55ad40c31a78) at /opt/glewlwyd/build/ulfius-src/src/ulfius.c:539 #14 0x00007fb3288a6c23 in ?? () from /usr/lib/libmicrohttpd.so.12 #15 0x00007fb3288a8a97 in ?? () from /usr/lib/libmicrohttpd.so.12 #16 0x00007fb3288a9b25 in ?? () from /usr/lib/libmicrohttpd.so.12 #17 0x00007fb3288aca12 in ?? () from /usr/lib/libmicrohttpd.so.12 #18 0x00007fb328e6933e in start (p=0x7fb3281438c0) at src/thread/pthread_create.c:192 #19 0x00007fb328e6b447 in __clone () at src/thread/x86_64/clone.s:22 ```

babelouest (Migrated from github.com) reviewed

2020-02-16 14:11:03 +01:00

src/misc.c

					
				@ -101,0 +98,4 @@

				    struct addrinfo *lookup;

				    int rv = getaddrinfo(ip_source, NULL, &hints, &lookup);

				    if (rv == 0) {

				      if (lookup->ai_canonname) {

Even though it's no longer mandatory, I'm used to declare all variables at the beginning of the functions, I find it easier to read and to miss multiple variables with the same name.