patch to fix bug in cscope feature
Dominique Pelle
Valgrind memory checker detects a bug when entering the command ":cs find g".
(requires support for cscope in vim)
==19660== Conditional jump or move depends on uninitialised value(s)
==19660== at 0x80D27F4: cs_find (if_cscope.c:969)
==19660== by 0x80D13E6: do_cscope_general (if_cscope.c:135)
==19660== by 0x80D1415: do_cscope (if_cscope.c:150)
==19660== by 0x809A1FB: do_one_cmd (ex_docmd.c:2622)
==19660== by 0x8097B18: do_cmdline (ex_docmd.c:1100)
==19660== by 0x8113F5C: nv_colon (normal.c:5168)
==19660== by 0x810E0B3: normal_cmd (normal.c:1141)
==19660== by 0x80D5939: main_loop (main.c:1180)
==19660== by 0x80D554D: main (main.c:939)
Relevant code in if_cscope.c is:
951 cs_find(eap)
952 exarg_T *eap;
953 {
954 char *opt, *pat;
955
956 if (cs_check_for_connections() == FALSE)
957 {
958 (void)EMSG(_("E567: no cscope connections"));
959 return FALSE;
960 }
961
962 if ((opt = strtok((char *)NULL, (const char *)" ")) == NULL)
963 {
964 cs_usage_msg(Find);
965 return FALSE;
966 }
967
968 pat = opt + strlen(opt) + 1;
!! 969 if (pat == NULL || (pat != NULL && pat[0] == '\0'))
970 {
971 cs_usage_msg(Find);
972 return FALSE;
973 }
974
975 return cs_find_common(opt, pat, eap->forceit, TRUE,
976 eap->cmdidx == CMD_lcscope);
977 } /* cs_find */
Vim accesses uninitialized memory after the string ":cs find g"
at line 969. As a result, it may display usage or may call
(incorrectly) cs_find_common().
Bug happens because command ":cs find g" is illegal (pattern
to search is missing). Correct command would be something like
":cs find some_pattern". However, entering an incorrect command
should not cause vim to to access uninitialized memory. It
should display the usage of command.
I attach a patch which fixes it. It calls strtok() another time
to find the pattern after "cs find g". Note that patch changes
the behavior slightly when entering several spaces before pattern.
With patch, ":cs find pattern" will search for "pattern".
Without patch, ":cs find pattern" will search for " pattern".
I'm not sure whether that's a better behavior or not. I personally
prefer behavior after patch. At least it fixes the access to
uninitialized memory. If original behavior must be preserved,
then bug needs to be fixed differently.
I was using vim-7.1 (patches 1-79) built with +cscope feature
on Linux x86.
-- Dominique
Dominique Pelle
in the patch I sent in my previous email.
So here it is again without the memory leak.
retrieving revision 1.22
diff -c -r1.22 if_cscope.c
*** if_cscope.c 11 Mar 2007 14:48:29 -0000 1.22
--- if_cscope.c 18 Aug 2007 17:48:54 -0000
***************
*** 952,957 ****
--- 952,958 ----
exarg_T *eap;
{
char *opt, *pat;
+ int jump;
if (cs_check_for_connections() == FALSE)
{
***************
*** 964,979 ****
cs_usage_msg(Find);
return FALSE;
}
! pat = opt + strlen(opt) + 1;
! if (pat == NULL || (pat != NULL && pat[0] == '\0'))
{
cs_usage_msg(Find);
return FALSE;
}
! return cs_find_common(opt, pat, eap->forceit, TRUE,
! eap->cmdidx == CMD_lcscope);
} /* cs_find */
--- 965,983 ----
cs_usage_msg(Find);
return FALSE;
}
+ opt = strdup(opt);
! if ((pat = strtok((char *)NULL, (const char *)" ")) == NULL)
{
cs_usage_msg(Find);
+ vim_free(opt);
return FALSE;
}
! jump = cs_find_common(opt, pat, eap->forceit, TRUE,
! eap->cmdidx == CMD_lcscope);
! vim_free(opt);
! return jump;
} /* cs_find */
Bram Moolenaar
Dominique Pelle wrote:
Thanks for locating this problem and suggesting a solution.
I don't think that skipping white space before the pattern is a problem.
I would rather call it an improvement. However, I think your solution
also has the effect that it's not possible to have a space in the
pattern. That is undesired.
Can you try that this is solved by passing an empty string to strtok()?
An alternative is to do away with strtok() and use skipwhite() and
skiptowhite(). That avoids the need to duplicate "opt".
--
Press any key to continue, press any other key to quit.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ download, build and distribute -- http://www.A-A-P.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Dominique Pelle
> I would rather call it an improvement. However, I think your solution
> also has the effect that it's not possible to have a space in the
> pattern. That is undesired.
I found another way to fix it, and preserve old behavior, so that it
allows to search for pattern which contains, start or ends with
space(s). For example ":cs find e a " will successfully match
strings " a " (space, a, space).
I attach the patch to this email. Patch also contain fixes for
a couple of typos.
-- Dominique
Bram Moolenaar
Dominique Pelle wrote:
Hmm, strtok() doesn't make it easy for us. Apparently there is no way
to get the pointer of where it currently is looking for the next token
without changing the string.
Your solution depends on strtok() changing the space after the command to
a NUL. What if there are several spaces: ":cs find e a"? I don't
think it is defined what strtok() does with the extra spaces. They
could be replaced with NUL or not.
Another solution, which is not clean either, is to store the length of
eap->arg in a global variable, before the first call to strtok().
--
hundred-and-one symptoms of being an internet addict:
196. Your computer costs more than your car.
Dominique Pelle
> a NUL. What if there are several spaces: ":cs find e a"? I don't
> think it is defined what strtok() does with the extra spaces. They
> could be replaced with NUL or not.
>
> Another solution, which is not clean either, is to store the length of
> eap->arg in a global variable, before the first call to strtok().
Yes. That assumption bothered me a bit too. Changing eap->arg
after call to strtok(eap->arg, " ") may be asking for trouble. But
information is just missing in cs_find() to implement a fix without
either:
- passing an extra parameter to the function (but it's used
as function pointer, so we'd need to change other functions
too (so I don't like it)
- adding a field in exarg_T with length of eap->arg, but type
is used in tons of other places (so I don't like it)
- using a global static variable as you suggest to store length
of eap->arg (maybe not clean, but most pragmatic solution)
So I attach a patch which uses a global static variable.
It is more portable and also simpler than my previous patch.
-- Dominique
Bram Moolenaar
Dominique Pelle wrote:
Thanks for continuing to improve the patch. It think this is the best
solution. I'll include it now.
- Bram
--
hundred-and-one symptoms of being an internet addict:
203. You're an active member of more than 20 newsgroups.