From 014febef22a279b9a457aa2830caeec1d9917461 Mon Sep 17 00:00:00 2001 From: Nicolas Hillegeer Date: Sat, 24 May 2014 15:09:09 +0200 Subject: coverity: fix BUFFER_SIZE_WARNING with str{n,l}cpy Relates to issue #760 These coverity warnings are of the form: >>> CID 62602: Buffer not null terminated (BUFFER_SIZE_WARNING) >>> Calling strncpy with a maximum size argument of 256 bytes... This is caused by strncpy not alway NULL-terminated the destination buffer (for example in the case where strlen(src) >= size(dst)). It's better to replace that with (x)strlcpy, which always NULL-terminates. Most of these are related to the set_api_error macro, which uses strncpy. The error struct is used (for example) in msgpack_rpc_error, where strlen is executed on it, so it needs to be NULL-terminated. (x)strlcpy, unlike strncpy, always NULL-terminates the destination buffer. Relevant parts of the coverity report: *** CID 62602: Buffer not null terminated (BUFFER_SIZE_WARNING) /src/nvim/api/vim.c: 236 in vim_set_current_buffer() 230 if (try_end(err)) { 231 return; 232 } 233 234 char msg[256]; 235 snprintf(msg, sizeof(msg), "failed to switch to buffer %d", (int)buffer); >>> CID 62602: Buffer not null terminated (BUFFER_SIZE_WARNING) >>> Calling strncpy with a maximum size argument of 256 bytes on >>> destination array "err->msg" of size 256 bytes might leave the >>> destination string unterminated. 236 set_api_error(msg, err); 237 return; 238 } 239 240 try_end(err); 241 } *** CID 62603: Buffer not null terminated (BUFFER_SIZE_WARNING) /src/nvim/api/private/helpers.c: 70 in try_end() 64 } else if (msg_list != NULL && *msg_list != NULL) { 65 int should_free; 66 char *msg = (char *)get_exception_string(*msg_list, 67 ET_ERROR, 68 NULL, 69 &should_free); >>> CID 62603: Buffer not null terminated (BUFFER_SIZE_WARNING) >>> Calling strncpy with a maximum size argument of 256 bytes on >>> destination array "err->msg" of size 256 bytes might leave the >>> destination string unterminated. 70 strncpy(err->msg, msg, sizeof(err->msg)); 71 err->set = true; 72 free_global_msglist(); 73 74 if (should_free) { 75 free(msg); /src/nvim/api/private/helpers.c: 78 in try_end() 72 free_global_msglist(); 73 74 if (should_free) { 75 free(msg); 76 } 77 } else if (did_throw) { >>> CID 62603: Buffer not null terminated (BUFFER_SIZE_WARNING) >>> Calling strncpy with a maximum size argument of 256 bytes on >>> destination array "err->msg" of size 256 bytes might leave the >>> destination string unterminated. 78 set_api_error((char *)current_exception->value, err); 79 } 80 81 return err->set; 82 } 83 *** CID 62604: Buffer not null terminated (BUFFER_SIZE_WARNING) /src/nvim/api/private/helpers.c: 592 in set_option_value_err() 586 opt_flags))) 587 { 588 if (try_end(err)) { 589 return; 590 } 591 >>> CID 62604: Buffer not null terminated (BUFFER_SIZE_WARNING) >>> Calling strncpy with a maximum size argument of 256 bytes on >>> destination array "err->msg" of size 256 bytes might leave the >>> destination string unterminated. 592 set_api_error(errmsg, err); 593 } *** CID 62605: Buffer not null terminated (BUFFER_SIZE_WARNING) /src/nvim/os/server.c: 114 in server_start() 108 if (addr_len > sizeof(ip) - 1) { 109 // Maximum length of a ip address buffer is 15(eg: 255.255.255.255) 110 addr_len = sizeof(ip); 111 } 112 113 // Extract the address part >>> CID 62605: Buffer not null terminated (BUFFER_SIZE_WARNING) >>> Calling strncpy with a maximum size argument of 16 bytes on >>> destination array "ip" of size 16 bytes might leave the destination >>> string unterminated. 114 strncpy(ip, addr, addr_len); 115 116 int port = NEOVIM_DEFAULT_TCP_PORT; 117 118 if (*ip_end == ':') { 119 char *port_end; /src/nvim/os/server.c: 88 in server_start() 82 83 void server_start(char *endpoint, ChannelProtocol prot) 84 { 85 char addr[ADDRESS_MAX_SIZE]; 86 87 // Trim to `ADDRESS_MAX_SIZE` >>> CID 62605: Buffer not null terminated (BUFFER_SIZE_WARNING) >>> Calling strncpy with a maximum size argument of 256 bytes on >>> destination array "addr" of size 256 bytes might leave the >>> destination string unterminated. 88 strncpy(addr, endpoint, sizeof(addr)); 89 90 // Check if the server already exists 91 if (map_has(cstr_t)(servers, addr)) { 92 EMSG2("Already listening on %s", addr); 93 return; *** CID 62606: Buffer not null terminated (BUFFER_SIZE_WARNING) /src/nvim/os/server.c: 186 in server_stop() 180 void server_stop(char *endpoint) 181 { 182 Server *server; 183 char addr[ADDRESS_MAX_SIZE]; 184 185 // Trim to `ADDRESS_MAX_SIZE` >>> CID 62606: Buffer not null terminated (BUFFER_SIZE_WARNING) >>> Calling strncpy with a maximum size argument of 256 bytes on >>> destination array "addr" of size 256 bytes might leave the >>> destination string unterminated. 187 188 if ((server = map_get(cstr_t)(servers, addr)) == NULL) { 189 EMSG2("Not listening on %s", addr); 190 return; 191 } --- src/nvim/api/private/helpers.c | 2 +- src/nvim/api/private/helpers.h | 3 ++- src/nvim/os/server.c | 12 ++++++++---- 3 files changed, 11 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/nvim/api/private/helpers.c b/src/nvim/api/private/helpers.c index c4340ddd89..a43e7a8d2a 100644 --- a/src/nvim/api/private/helpers.c +++ b/src/nvim/api/private/helpers.c @@ -67,7 +67,7 @@ bool try_end(Error *err) ET_ERROR, NULL, &should_free); - strncpy(err->msg, msg, sizeof(err->msg)); + xstrlcpy(err->msg, msg, sizeof(err->msg)); err->set = true; free_global_msglist(); diff --git a/src/nvim/api/private/helpers.h b/src/nvim/api/private/helpers.h index da7e357d04..2d917c2b5e 100644 --- a/src/nvim/api/private/helpers.h +++ b/src/nvim/api/private/helpers.h @@ -5,10 +5,11 @@ #include "nvim/api/private/defs.h" #include "nvim/vim.h" +#include "nvim/memory.h" #define set_api_error(message, err) \ do { \ - strncpy(err->msg, message, sizeof(err->msg)); \ + xstrlcpy(err->msg, message, sizeof(err->msg)); \ err->set = true; \ } while (0) diff --git a/src/nvim/os/server.c b/src/nvim/os/server.c index 9b5410c323..b2faa49a86 100644 --- a/src/nvim/os/server.c +++ b/src/nvim/os/server.c @@ -85,7 +85,11 @@ void server_start(char *endpoint, ChannelProtocol prot) char addr[ADDRESS_MAX_SIZE]; // Trim to `ADDRESS_MAX_SIZE` - strncpy(addr, endpoint, sizeof(addr)); + if (xstrlcpy(addr, endpoint, sizeof(addr)) >= sizeof(addr)) { + // TODO(aktau): since this is not what the user wanted, perhaps we + // should return an error here + EMSG2("Address was too long, truncated to %s", addr); + } // Check if the server already exists if (map_has(cstr_t)(servers, addr)) { @@ -111,7 +115,7 @@ void server_start(char *endpoint, ChannelProtocol prot) } // Extract the address part - strncpy(ip, addr, addr_len); + xstrlcpy(ip, addr, addr_len); int port = NEOVIM_DEFAULT_TCP_PORT; @@ -119,7 +123,7 @@ void server_start(char *endpoint, ChannelProtocol prot) char *port_end; // Extract the port port = strtol(ip_end + 1, &port_end, 10); - + errno = 0; if (errno != 0 || port == 0 || port > 0xffff) { // Invalid port, treat as named pipe or unix socket @@ -183,7 +187,7 @@ void server_stop(char *endpoint) char addr[ADDRESS_MAX_SIZE]; // Trim to `ADDRESS_MAX_SIZE` - strncpy(addr, endpoint, sizeof(addr)); + xstrlcpy(addr, endpoint, sizeof(addr)); if ((server = map_get(cstr_t)(servers, addr)) == NULL) { EMSG2("Not listening on %s", addr); -- cgit