Skip to content

Commit b1dddb5

Browse files
addressed possible null pointer dereferences (coturn#1729)
this pr aims to address more static code analyser warnings, specifically null pointer dereferences. the majority of changes are solely to quieten the analyser, as `malloc` and `calloc` are unlikely to fail, but this should at least lead to the code analysis being more readable and usable. where functions addressed had existing failure strategies, they were used, however some functions will now silently fail rather than attempting to dereference a null pointer. if there is a preferred solution in these cases, i will be happy to implement it. --- - [27](https://github.com/redraincatching/coturn/security/code-scanning/27): moved use of pointer inside `else` block of null check - [42](https://github.com/redraincatching/coturn/security/code-scanning/42): added early return in case of null pointer - [69](https://github.com/redraincatching/coturn/security/code-scanning/69): added null pointer check after `malloc` - [76](https://github.com/redraincatching/coturn/security/code-scanning/76): added null pointer check after `calloc` - [77](https://github.com/redraincatching/coturn/security/code-scanning/77): added null pointer check to loop guard - [82](https://github.com/redraincatching/coturn/security/code-scanning/82): added null pointer check after `malloc` - [83](https://github.com/redraincatching/coturn/security/code-scanning/83): added null pointer check after `malloc` - [84](https://github.com/redraincatching/coturn/security/code-scanning/84): added null pointer check after `calloc` - [85](https://github.com/redraincatching/coturn/security/code-scanning/85): added null pointer check around pointer use, as done earlier in the same function - [86](https://github.com/redraincatching/coturn/security/code-scanning/86): added null pointer check after `calloc` - [90](https://github.com/redraincatching/coturn/security/code-scanning/90)/[91](https://github.com/redraincatching/coturn/security/code-scanning/91)/[92](https://github.com/redraincatching/coturn/security/code-scanning/92)/[93](https://github.com/redraincatching/coturn/security/code-scanning/93): added null pointer check to block - [94](https://github.com/redraincatching/coturn/security/code-scanning/94)/[95](https://github.com/redraincatching/coturn/security/code-scanning/95): added null pointer checks after `malloc` - [108](https://github.com/redraincatching/coturn/security/code-scanning/108): added check after `calloc` - [114](https://github.com/redraincatching/coturn/security/code-scanning/114): added check after `memcpy` - [129](https://github.com/redraincatching/coturn/security/code-scanning/129): added check after `calloc` - [145](https://github.com/redraincatching/coturn/security/code-scanning/145): added check to if guard - [146](https://github.com/redraincatching/coturn/security/code-scanning/146): added check to if guard - [154](https://github.com/redraincatching/coturn/security/code-scanning/154): added early exit with error - [165](https://github.com/redraincatching/coturn/security/code-scanning/165): added check after `malloc` - [170](https://github.com/redraincatching/coturn/security/code-scanning/170): added early null return on null pointer - [171](https://github.com/redraincatching/coturn/security/code-scanning/171): added check after `calloc` --- ![You're dereferencing a null pointer!](https://i.makeagif.com/media/9-29-2015/YwGqu_.gif)
1 parent 0f46392 commit b1dddb5

7 files changed

Lines changed: 40 additions & 26 deletions

File tree

src/apps/common/apputils.c

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -919,6 +919,9 @@ static char *_WTA(__in wchar_t *pszInBuf, __in int nInSize, __out char **pszOutB
919919
// and we have to add space to the allocation ourselves.
920920
(*pnOutSize)++;
921921
*pszOutBuf = malloc(*pnOutSize * sizeof(char));
922+
if (!pszOutBuf) {
923+
return NULL;
924+
}
922925
if (WideCharToMultiByte((UINT)0, (DWORD)0, pszInBuf, nInSize, *pszOutBuf, *pnOutSize, NULL, NULL) == 0) {
923926
free(*pszOutBuf);
924927
return NULL;
@@ -1340,10 +1343,12 @@ void build_base64_decoding_table(void) {
13401343

13411344
char *table = (char *)calloc(256, sizeof(char));
13421345

1343-
for (size_t i = 0; i < 64; i++) {
1344-
table[(unsigned char)encoding_table[i]] = i;
1346+
if (table) {
1347+
for (size_t i = 0; i < 64; i++) {
1348+
table[(unsigned char)encoding_table[i]] = i;
1349+
}
1350+
decoding_table = table;
13451351
}
1346-
decoding_table = table;
13471352
}
13481353

13491354
unsigned char *base64_decode(const char *data, size_t input_length, size_t *output_length) {

src/apps/relay/ns_ioalib_engine_impl.c

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -594,11 +594,13 @@ ioa_timer_handle set_ioa_timer(ioa_engine_handle e, int secs, int ms, ioa_timer_
594594

595595
tv.tv_sec = secs;
596596

597-
te->ctx = ctx;
598-
te->e = e;
599-
te->ev = ev;
600-
te->cb = cb;
601-
te->txt = strdup(txt);
597+
if (te) {
598+
te->ctx = ctx;
599+
te->e = e;
600+
te->ev = ev;
601+
te->cb = cb;
602+
te->txt = strdup(txt);
603+
}
602604

603605
if (!ms) {
604606
tv.tv_usec = 0;
@@ -1364,7 +1366,6 @@ ioa_socket_handle create_ioa_socket_from_fd(ioa_engine_handle e, ioa_socket_raw
13641366
}
13651367

13661368
ret->magic = SOCKET_MAGIC;
1367-
13681369
ret->fd = fd;
13691370
ret->st = st;
13701371
ret->sat = sat;
@@ -3661,7 +3662,7 @@ void turn_report_allocation_set(void *a, turn_time_t lifetime, int refresh) {
36613662
}
36623663
}
36633664
#if !defined(TURN_NO_HIREDIS)
3664-
if (e && ss) {
3665+
if (e && ss && ss->client_socket) {
36653666
char key[1024];
36663667
if (ss->realm_options.name[0]) {
36673668
snprintf(key, sizeof(key), "turn/realm/%s/user/%s/allocation/%018llu/status", ss->realm_options.name,

src/apps/relay/turn_admin_server.c

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,20 +1178,22 @@ static void cliserver_input_handler(struct evconnlistener *l, evutil_socket_t fd
11781178
return;
11791179
}
11801180

1181-
clisession->rp = get_realm(NULL);
1181+
if (clisession) {
1182+
clisession->rp = get_realm(NULL);
11821183

1183-
set_socket_options_fd(fd, TCP_SOCKET, sa->sa_family);
1184+
set_socket_options_fd(fd, TCP_SOCKET, sa->sa_family);
11841185

1185-
clisession->fd = fd;
1186+
clisession->fd = fd;
11861187

1187-
addr_cpy(&(clisession->addr), (ioa_addr *)sa);
1188+
addr_cpy(&(clisession->addr), (ioa_addr *)sa);
11881189

1189-
clisession->bev = bufferevent_socket_new(adminserver.event_base, fd, TURN_BUFFEREVENTS_OPTIONS);
1190-
bufferevent_setcb(clisession->bev, cli_socket_input_handler_bev, NULL, cli_eventcb_bev, clisession);
1191-
bufferevent_setwatermark(clisession->bev, EV_READ | EV_WRITE, 0, BUFFEREVENT_HIGH_WATERMARK);
1192-
bufferevent_enable(clisession->bev, EV_READ); /* Start reading. */
1190+
clisession->bev = bufferevent_socket_new(adminserver.event_base, fd, TURN_BUFFEREVENTS_OPTIONS);
1191+
bufferevent_setcb(clisession->bev, cli_socket_input_handler_bev, NULL, cli_eventcb_bev, clisession);
1192+
bufferevent_setwatermark(clisession->bev, EV_READ | EV_WRITE, 0, BUFFEREVENT_HIGH_WATERMARK);
1193+
bufferevent_enable(clisession->bev, EV_READ); /* Start reading. */
11931194

1194-
clisession->ts = telnet_init(cli_telopts, cli_telnet_event_handler, 0, clisession);
1195+
clisession->ts = telnet_init(cli_telopts, cli_telnet_event_handler, 0, clisession);
1196+
}
11951197

11961198
if (!(clisession->ts)) {
11971199
const char *str = "Cannot open telnet session\n";
@@ -1449,7 +1451,7 @@ void admin_server_receive_message(struct bufferevent *bev, void *ptr) {
14491451
int n = 0;
14501452
struct evbuffer *input = bufferevent_get_input(bev);
14511453

1452-
while ((n = evbuffer_remove(input, tsi, sizeof(struct turn_session_info))) > 0) {
1454+
while (tsi && (n = evbuffer_remove(input, tsi, sizeof(struct turn_session_info))) > 0) {
14531455
if (n != sizeof(struct turn_session_info)) {
14541456
fprintf(stderr, "%s: Weird CLI buffer error: size=%d\n", __FUNCTION__, n);
14551457
continue;

src/apps/uclient/startuclient.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,7 @@ static int clnet_connect(uint16_t clnet_remote_port, const char *remote_address,
289289
STRCPY(clnet_info->ifname, (const char *)ifname);
290290
}
291291

292-
if (use_secure) {
292+
if (use_secure && clnet_info) {
293293
bool try_again = false;
294294
clnet_info->ssl = tls_connect(clnet_info->fd, &remote_addr, &try_again, connect_cycle++);
295295
if (!clnet_info->ssl) {

src/apps/uclient/uclient.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1489,7 +1489,7 @@ void start_mclient(const char *remote_address, int port, const unsigned char *if
14891489

14901490
if (is_TCP_relay()) {
14911491
if (passive_tcp) {
1492-
if (elems[i]->pinfo.is_peer) {
1492+
if (elems && elems[i]->pinfo.is_peer) {
14931493
int connect_err = 0;
14941494
socket_connect(elems[i]->pinfo.fd, &(elems[i]->pinfo.remote_addr), &connect_err);
14951495
}

src/client/ns_turn_msg.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,10 @@ bool stun_produce_integrity_key_str(const uint8_t *uname, const uint8_t *realm,
185185
return false;
186186
}
187187

188+
if (!str) {
189+
return false;
190+
}
191+
188192
strncpy((char *)str, (const char *)uname, sz);
189193
str[ulen] = ':';
190194
strncpy((char *)str + ulen + 1, (const char *)realm, sz - ulen - 1);

src/server/ns_turn_maps.c

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -951,13 +951,15 @@ static bool ur_string_map_init(ur_string_map *map) {
951951

952952
ur_string_map *ur_string_map_create(ur_string_map_func del_value_func) {
953953
ur_string_map *map = (ur_string_map *)malloc(sizeof(ur_string_map));
954-
if (!ur_string_map_init(map)) {
954+
if (map == NULL) {
955+
return NULL;
956+
} else if (!ur_string_map_init(map)) {
955957
free(map);
956958
return NULL;
959+
} else {
960+
map->del_value_func = del_value_func;
961+
return map;
957962
}
958-
assert(map);
959-
map->del_value_func = del_value_func;
960-
return map;
961963
}
962964

963965
bool ur_string_map_put(ur_string_map *map, const ur_string_map_key_type key, ur_string_map_value_type value) {

0 commit comments

Comments
 (0)