From 7a8e41e8ee56a1fcec07f85566022548d41a944a Mon Sep 17 00:00:00 2001 From: oni-link Date: Thu, 21 Jan 2016 16:07:13 +0100 Subject: coverity/13753: Dereference null return value Dereferencing a pointer that might be null(ptag) when calling strlen(). False positive. A match always contains a tab, so ptag is never null. Because matches are always in ctags style, we can rewrite the code to not use strtok(). --- src/nvim/if_cscope.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) (limited to 'src') diff --git a/src/nvim/if_cscope.c b/src/nvim/if_cscope.c index d236501b3f..af3a87211a 100644 --- a/src/nvim/if_cscope.c +++ b/src/nvim/if_cscope.c @@ -1632,35 +1632,43 @@ static char *cs_pathcomponents(char *path) return s; } -/* - * PRIVATE: cs_print_tags_priv - * - * called from cs_manage_matches() - */ +/// Print cscope output that was converted into ctags style entries. +/// +/// Only called from cs_manage_matches(). +/// +/// @param matches Array of cscope lines in ctags style. Every entry was +// produced with a format string of the form +// "%s\t%s\t%s;\"\t%s" or +// "%s\t%s\t%s;\"" +// by cs_make_vim_style_matches(). +/// @param cntxts Context for matches. +/// @param num_matches Number of entries in matches/cntxts, always greater 0. static void cs_print_tags_priv(char **matches, char **cntxts, - size_t num_matches) + size_t num_matches) FUNC_ATTR_NONNULL_ALL { - char *ptag; char *fname, *lno, *extra, *tbuf; size_t num; char *globalcntx = "GLOBAL"; char *context; char *cstag_msg = _("Cscope tag: %s"); - assert (num_matches > 0); - - tbuf = xmalloc(strlen(matches[0]) + 1); + assert(num_matches > 0); + assert(strcnt(matches[0], '\t') >= 2); - strcpy(tbuf, matches[0]); - ptag = strtok(tbuf, "\t"); + char *ptag = matches[0]; + char *ptag_end = strchr(ptag, '\t'); + assert(ptag_end >= ptag); + // NUL terminate tag string in matches[0]. + *ptag_end = NUL; - size_t newsize = strlen(cstag_msg) + strlen(ptag); + size_t newsize = strlen(cstag_msg) + (size_t)(ptag_end - ptag); char *buf = xmalloc(newsize); size_t bufsize = newsize; // Track available bufsize (void)sprintf(buf, cstag_msg, ptag); MSG_PUTS_ATTR(buf, hl_attr(HLF_T)); - xfree(tbuf); + // restore matches[0] + *ptag_end = '\t'; MSG_PUTS_ATTR(_("\n # line"), hl_attr(HLF_T)); /* strlen is 7 */ msg_advance(msg_col + 2); @@ -1668,6 +1676,7 @@ static void cs_print_tags_priv(char **matches, char **cntxts, num = 1; for (size_t i = 0; i < num_matches; i++) { + assert(strcnt(matches[i], '\t') >= 2); size_t idx = i; /* if we really wanted to, we could avoid this malloc and strcpy -- cgit From f2558890f54722ab31b1e073e9ce844750cd200a Mon Sep 17 00:00:00 2001 From: oni-link Date: Thu, 21 Jan 2016 16:17:57 +0100 Subject: coverity/133858: Out-of-bounds access Allocating insufficient memory for the terminating NUL of the string. False positive, we allocating more memory than we need. --- src/nvim/if_cscope.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src') diff --git a/src/nvim/if_cscope.c b/src/nvim/if_cscope.c index af3a87211a..476dcc9478 100644 --- a/src/nvim/if_cscope.c +++ b/src/nvim/if_cscope.c @@ -1661,6 +1661,8 @@ static void cs_print_tags_priv(char **matches, char **cntxts, // NUL terminate tag string in matches[0]. *ptag_end = NUL; + // The "%s" in cstag_msg won't appear in the result string, so we don't need + // extra memory for terminating NUL. size_t newsize = strlen(cstag_msg) + (size_t)(ptag_end - ptag); char *buf = xmalloc(newsize); size_t bufsize = newsize; // Track available bufsize -- cgit From a649299e76cb1a30cb3388a4bf1fe0410022374d Mon Sep 17 00:00:00 2001 From: oni-link Date: Thu, 21 Jan 2016 16:22:34 +0100 Subject: coverity/133892: Resource leak Variable tbuf going out of scope leaks the storage it points to. We don't have to use the copy tbuf of a match. Because matches are always in ctags style, we can operate on them directly. --- src/nvim/if_cscope.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) (limited to 'src') diff --git a/src/nvim/if_cscope.c b/src/nvim/if_cscope.c index 476dcc9478..347f214206 100644 --- a/src/nvim/if_cscope.c +++ b/src/nvim/if_cscope.c @@ -1646,7 +1646,6 @@ static char *cs_pathcomponents(char *path) static void cs_print_tags_priv(char **matches, char **cntxts, size_t num_matches) FUNC_ATTR_NONNULL_ALL { - char *fname, *lno, *extra, *tbuf; size_t num; char *globalcntx = "GLOBAL"; char *context; @@ -1681,26 +1680,23 @@ static void cs_print_tags_priv(char **matches, char **cntxts, assert(strcnt(matches[i], '\t') >= 2); size_t idx = i; - /* if we really wanted to, we could avoid this malloc and strcpy - * by parsing matches[i] on the fly and placing stuff into buf - * directly, but that's too much of a hassle - */ - tbuf = xmalloc(strlen(matches[idx]) + 1); - (void)strcpy(tbuf, matches[idx]); - - if (strtok(tbuf, (const char *)"\t") == NULL) - continue; - if ((fname = strtok(NULL, (const char *)"\t")) == NULL) - continue; - if ((lno = strtok(NULL, (const char *)"\t")) == NULL) - continue; - extra = strtok(NULL, (const char *)"\t"); + // Parse filename, line number and optional part. + char *fname = strchr(matches[i], '\t') + 1; + char *fname_end = strchr(fname, '\t'); + // Replace second '\t' in matches[i] with NUL to terminate fname. + *fname_end = NUL; - lno[strlen(lno)-2] = '\0'; /* ignore ;" at the end */ + char *lno = fname_end + 1; + char *extra = xstrchrnul(lno, '\t'); + // Ignore ;" at the end of lno. + char *lno_end = extra - 2; + *lno_end = NUL; + // Do we have an optional part? + extra = *extra ? extra + 1 : NULL; const char *csfmt_str = "%4zu %6s "; /* hopefully 'num' (num of matches) will be less than 10^16 */ - newsize = strlen(csfmt_str) + 16 + strlen(lno); + newsize = strlen(csfmt_str) + 16 + (size_t)(lno_end - lno); if (bufsize < newsize) { buf = xrealloc(buf, newsize); bufsize = newsize; @@ -1737,7 +1733,9 @@ static void cs_print_tags_priv(char **matches, char **cntxts, MSG_PUTS_LONG(extra); } - xfree(tbuf); /* only after printing extra due to strtok use */ + // restore matches[i] + *fname_end = '\t'; + *lno_end = ';'; if (msg_col) msg_putchar('\n'); -- cgit From dd0b358af5b3f99f861c99f4ba2d0c9ad16aafb1 Mon Sep 17 00:00:00 2001 From: oni-link Date: Thu, 21 Jan 2016 16:42:27 +0100 Subject: cs_print_tags_priv: Clear first output line to screen end Using `:cscope find s ` with the cursor on a very short word like `key` does not output the first line on the screen correctly: Output is `Cscope tag: keyrd>` instead of `Cscope tag: key`. To fix this, clear the screen line after the first line was printed. --- src/nvim/if_cscope.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src') diff --git a/src/nvim/if_cscope.c b/src/nvim/if_cscope.c index 347f214206..64c381cda3 100644 --- a/src/nvim/if_cscope.c +++ b/src/nvim/if_cscope.c @@ -1667,6 +1667,7 @@ static void cs_print_tags_priv(char **matches, char **cntxts, size_t bufsize = newsize; // Track available bufsize (void)sprintf(buf, cstag_msg, ptag); MSG_PUTS_ATTR(buf, hl_attr(HLF_T)); + msg_clr_eos(); // restore matches[0] *ptag_end = '\t'; -- cgit From 15cd8916df17da1906180cd6892fb97d0f6aacd2 Mon Sep 17 00:00:00 2001 From: oni-link Date: Thu, 21 Jan 2016 17:02:09 +0100 Subject: cs_print_tags_priv: Clean up function. * Style changes * Variable removal * Comment update --- src/nvim/if_cscope.c | 44 +++++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 25 deletions(-) (limited to 'src') diff --git a/src/nvim/if_cscope.c b/src/nvim/if_cscope.c index 64c381cda3..7169a1d963 100644 --- a/src/nvim/if_cscope.c +++ b/src/nvim/if_cscope.c @@ -1646,10 +1646,8 @@ static char *cs_pathcomponents(char *path) static void cs_print_tags_priv(char **matches, char **cntxts, size_t num_matches) FUNC_ATTR_NONNULL_ALL { - size_t num; - char *globalcntx = "GLOBAL"; - char *context; - char *cstag_msg = _("Cscope tag: %s"); + char *globalcntx = "GLOBAL"; + char *cstag_msg = _("Cscope tag: %s"); assert(num_matches > 0); assert(strcnt(matches[0], '\t') >= 2); @@ -1665,21 +1663,20 @@ static void cs_print_tags_priv(char **matches, char **cntxts, size_t newsize = strlen(cstag_msg) + (size_t)(ptag_end - ptag); char *buf = xmalloc(newsize); size_t bufsize = newsize; // Track available bufsize - (void)sprintf(buf, cstag_msg, ptag); + (void)snprintf(buf, bufsize, cstag_msg, ptag); MSG_PUTS_ATTR(buf, hl_attr(HLF_T)); msg_clr_eos(); // restore matches[0] *ptag_end = '\t'; - MSG_PUTS_ATTR(_("\n # line"), hl_attr(HLF_T)); /* strlen is 7 */ + // Column headers for match number, line number and filename. + MSG_PUTS_ATTR(_("\n # line"), hl_attr(HLF_T)); msg_advance(msg_col + 2); MSG_PUTS_ATTR(_("filename / context / line\n"), hl_attr(HLF_T)); - num = 1; for (size_t i = 0; i < num_matches; i++) { assert(strcnt(matches[i], '\t') >= 2); - size_t idx = i; // Parse filename, line number and optional part. char *fname = strchr(matches[i], '\t') + 1; @@ -1696,21 +1693,18 @@ static void cs_print_tags_priv(char **matches, char **cntxts, extra = *extra ? extra + 1 : NULL; const char *csfmt_str = "%4zu %6s "; - /* hopefully 'num' (num of matches) will be less than 10^16 */ + // hopefully num_matches will be less than 10^16 newsize = strlen(csfmt_str) + 16 + (size_t)(lno_end - lno); if (bufsize < newsize) { buf = xrealloc(buf, newsize); bufsize = newsize; } - (void)sprintf(buf, csfmt_str, num, lno); + (void)snprintf(buf, bufsize, csfmt_str, i + 1, lno); MSG_PUTS_ATTR(buf, hl_attr(HLF_CM)); MSG_PUTS_LONG_ATTR(cs_pathcomponents(fname), hl_attr(HLF_CM)); - /* compute the required space for the context */ - if (cntxts[idx] != NULL) - context = cntxts[idx]; - else - context = globalcntx; + // compute the required space for the context + char *context = cntxts[i] ? cntxts[i] : globalcntx; const char *cntxformat = " <<%s>>"; // '%s' won't appear in result string, so: @@ -1721,11 +1715,13 @@ static void cs_print_tags_priv(char **matches, char **cntxts, buf = xrealloc(buf, newsize); bufsize = newsize; } - (void)sprintf(buf, cntxformat, context); + int buf_len = snprintf(buf, bufsize, cntxformat, context); + assert(buf_len >= 0); - /* print the context only if it fits on the same line */ - if (msg_col + (int)strlen(buf) >= (int)Columns) + // Print the context only if it fits on the same line. + if (msg_col + buf_len >= (int)Columns) { msg_putchar('\n'); + } msg_advance(12); MSG_PUTS_LONG(buf); msg_putchar('\n'); @@ -1738,21 +1734,19 @@ static void cs_print_tags_priv(char **matches, char **cntxts, *fname_end = '\t'; *lno_end = ';'; - if (msg_col) + if (msg_col) { msg_putchar('\n'); + } os_breakcheck(); if (got_int) { - got_int = FALSE; /* don't print any more matches */ + got_int = false; // don't print any more matches break; } - - num++; - } /* for all matches */ + } xfree(buf); -} /* cs_print_tags_priv */ - +} /* * PRIVATE: cs_read_prompt -- cgit