From a50a34f4723b23d1846f7b71c4584dbda7cf579a Mon Sep 17 00:00:00 2001 From: Nicolas Hillegeer Date: Sat, 24 May 2014 15:02:13 +0200 Subject: memory: add xstrlcpy Less "blow a hole in your foot" than strncpy. As also indicated by coverity. Implementation inspired by the linux kernel (very similar to OSX's Libc implementation as well). --- src/nvim/memory.c | 13 +++++++++++++ src/nvim/memory.h | 13 +++++++++++++ 2 files changed, 26 insertions(+) (limited to 'src') diff --git a/src/nvim/memory.c b/src/nvim/memory.c index b6990890a6..238a6791c0 100644 --- a/src/nvim/memory.c +++ b/src/nvim/memory.c @@ -183,6 +183,19 @@ char *xstpncpy(char *restrict dst, const char *restrict src, size_t maxlen) } } +size_t xstrlcpy(char *restrict dst, const char *restrict src, size_t size) +{ + size_t ret = strlen(src); + + if (size) { + size_t len = (ret >= size) ? size - 1 : ret; + memcpy(dst, src, len); + dst[len] = '\0'; + } + + return ret; +} + char *xstrdup(const char *str) { char *ret = strdup(str); diff --git a/src/nvim/memory.h b/src/nvim/memory.h index 2723a7ba80..accf293176 100644 --- a/src/nvim/memory.h +++ b/src/nvim/memory.h @@ -125,6 +125,19 @@ char *xstpcpy(char *restrict dst, const char *restrict src); /// @param maxlen char *xstpncpy(char *restrict dst, const char *restrict src, size_t maxlen); +/// xstrlcpy - Copy a %NUL terminated string into a sized buffer +/// +/// Compatible with *BSD strlcpy: the result is always a valid +/// NUL-terminated string that fits in the buffer (unless, +/// of course, the buffer size is zero). It does not pad +/// out the result like strncpy() does. +/// +/// @param dst Where to copy the string to +/// @param src Where to copy the string from +/// @param size Size of destination buffer +/// @return Length of the source string (i.e.: strlen(src)) +size_t xstrlcpy(char *restrict dst, const char *restrict src, size_t size); + /// Duplicates a chunk of memory using xmalloc /// /// @see {xmalloc} -- cgit 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