Dominique Pelle wrote:
> I notice that Vim-7.2.40 (huge) crashes on start up when I
> compile it with gcc 4.3.2 with -O3 (that's the default gcc
> version from Ubuntu-8.10), but it works perfectly fine when
> compiled with -O0, -O1 or -O2.
>
> Here is the crash:
[...]
> **7840** *** strcpy_chk: buffer overflow detected ***: program terminated
> ==7840== at 0x4027871: VALGRIND_PRINTF_BACKTRACE (valgrind.h:3695)
> ==7840== by 0x4027A37: __strcpy_chk (mc_replace_strmem.c:614)
> ==7840== by 0x80817DE: call_user_func (string3.h:106)
> ==7840== by 0x8082ED0: call_func (eval.c:8017)
[...]
> Adding some printf(), I see that Vim crashes at line 22154 in eval.c
> in the STRCPY(...):
>
> 21153 v = &fc.fixvar[fixvar_idx++].var;
> 21154 STRCPY(v->di_key, "000");
> 21155 v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX;
>
> v is declared as: 'dictitem_T *v;' with:
>
> struct dictitem_S
> {
> typval_T di_tv; /* type and value of the variable */
> char_u di_flags; /* flags (only used for variable) */
> char_u di_key[1]; /* key (actually longer!) */
> };
>
> typedef struct dictitem_S dictitem_T;
>
>
> So copying "000" in 'char_u di_key[1]' is of course a bit special
> but it's not a bug I think, it's a classic pattern described in:
>
> http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Zero-Length.html#Zero-Length
>
> There should in practice be no corruption since:
>
> v = &fc.fixvar[fixvar_idx++].var;
>
> ... with fixvar defined as:
>
> 227 struct /* fixed variables for arguments */
> 228 {
> 229 dictitem_T var; /* variable (without
> room for name) */
> 230 char_u room[VAR_SHORT_LEN]; /* room for the name */
> 231 } fixvar[FIXVAR_CNT];
>
> .... with FIXVAR_CNT being defined as #define FIXVAR_CNT 12
And VAR_SHORT_LEN is 20, so there are 21 bytes available for the key
name. A bit more if the compiler rounds off struct sizes.
> This code looks OK to me. Maybe it's a bug in gcc or maybe this
> construction is just not portable. I suspect it's a bug in gcc but
> I'm not 100% sure.
>
> In any case, I found that gcc has an extension to the C language
> which allows to declare the di_key array without a size:
>
> struct dictitem_S
> {
> typval_T di_tv; /* type and value of the variable */
> char_u di_flags; /* flags (only used for variable) */
> #ifdef __GNUC__
> /* Declaring di_key[] instead of di_key[1] prevents crashes when
> * compiling with gcc -O3 */
> char_u di_key[]; /* key (actually longer!) */
> #else
> char_u di_key[1]; /* key (actually longer!) */
> #endif
> };
>
>
> Doing that is enough to make it work with -O3, and also pacifies
> valgrind memory checker. I guess gcc then relaxes the optimization
> for di_key.
Unfortunately this solution only works for the GNU compiler, requiring
#ifdefs.
> There are other places (which I did not change yet) where
> the same problem could possibly happen. I see at least those:
>
> $ egrep -n "\[1\].*actually longer" *.c *.h
> eval.c:184: char_u uf_name[1]; /* name of function (actually longer);
> can
> memline.c:92: PTR_EN pb_pointer[1]; /* list of pointers to blocks
> (actually longer)
> message.c:2164: char_u sb_text[1]; /* text to be displayed,
> actually longer */
> misc2.c:3862: char_u ffv_fname[1]; /* actually longer */
> spell.c:582: char_u wc_word[1]; /* word, actually longer */
> spell.c:4801: char_u sb_data[1]; /* data, actually longer */
> spell.c:13081: char_u sft_word[1]; /* soundfolded word, actually
> longer */
> tag.c:1336: char_u match[1]; /* actually longer */
> regexp.h:37: char_u program[1]; /* actually
> longer.. */
> structs.h:414: char_u b_str[1]; /* contents (actually
> longer) */
> structs.h:768: char_u keyword[1]; /* actually longer */
> structs.h:1101: char_u di_key[1]; /* key (actually longer!) */
>
>
> I did not fix all those other places, but if you think that the
> propose way to fix is good, I can put it in all those places.
I don't think we need the fix in all those places, as it is only
triggered by using strcpy copying into that space.
Another solution is to use the trick what's done for "self" a few lines
higher:
v = &fc.fixvar[fixvar_idx++].var;
name = v->di_key;
STRCPY(name, "self");
So we can do the same for "000":
v = &fc.fixvar[fixvar_idx++].var;
name = v->di_key;
STRCPY(name, "000");
Can you please verify this also fixes the crash?
> I did not see this bug when using prior version of Ubuntu-8.04.1
> (can't remember which version of gcc it used)
>
> Interestingly, compiling with -O2 -g -finline-functions also gives
> the following warning:
>
> gcc -c -I. -Iproto -DHAVE_CONFIG_H -DFEAT_GUI_GTK
> -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include
> -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0
> -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include
> -I/usr/include/pixman-1 -I/usr/include/freetype2
> -I/usr/include/libpng12 -O2 -g -finline-functions -o
> objects/eval.o eval.c
> In function 'strcpy',
> inlined from 'add_nr_var' at eval.c:21397,
> inlined from 'call_user_func' at eval.c:21151:
> /usr/include/bits/string3.h:106: warning: call to
> __builtin___strcpy_chk will always overflow destination buffer
> In function 'strcpy',
> inlined from 'add_nr_var' at eval.c:21397,
> inlined from 'call_user_func' at eval.c:21169:
> /usr/include/bits/string3.h:106: warning: call to
> __builtin___strcpy_chk will always overflow destination buffer
> In function 'strcpy',
> inlined from 'add_nr_var' at eval.c:21397,
> inlined from 'call_user_func' at eval.c:21171:
> /usr/include/bits/string3.h:106: warning: call to
> __builtin___strcpy_chk will always overflow destination buffer
These all look lik add_nr_var() calls. I'll change the strcpy() there
as well.
So this is my patch. Let me know if this fixes it with your compiler:
*** ../vim-7.2.040/src/eval.c Wed Nov 12 15:28:37 2008
--- src/eval.c Sat Nov 15 12:28:32 2008
***************
*** 21150,21157 ****
init_var_dict(&fc.l_avars, &fc.l_avars_var);
add_nr_var(&fc.l_avars, &fc.fixvar[fixvar_idx++].var, "0",
(varnumber_T)(argcount - fp->uf_args.ga_len));
v = &fc.fixvar[fixvar_idx++].var;
! STRCPY(v->di_key, "000");
v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX;
hash_add(&fc.l_avars.dv_hashtab, DI2HIKEY(v));
v->di_tv.v_type = VAR_LIST;
--- 21150,21160 ----
init_var_dict(&fc.l_avars, &fc.l_avars_var);
add_nr_var(&fc.l_avars, &fc.fixvar[fixvar_idx++].var, "0",
(varnumber_T)(argcount - fp->uf_args.ga_len));
+ /* Use "name" to avoid a warning from some compiler that checks the
+ * destination size. */
v = &fc.fixvar[fixvar_idx++].var;
! name = v->di_key;
! STRCPY(name, "000");
v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX;
hash_add(&fc.l_avars.dv_hashtab, DI2HIKEY(v));
v->di_tv.v_type = VAR_LIST;
***************
*** 21394,21400 ****
char *name;
varnumber_T nr;
{
! STRCPY(v->di_key, name);
v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX;
hash_add(&dp->dv_hashtab, DI2HIKEY(v));
v->di_tv.v_type = VAR_NUMBER;
--- 21397,21409 ----
char *name;
varnumber_T nr;
{
! char *nname;
!
! /* When inlining and the compiler adds a check for the destination of
! * strcpy() it may complain about di_key being too small. Happens with
! * GCC 4.3.2 with -O3. Use an intermediate variable to avoid that. */
! nname = v->di_key;
! STRCPY(nname, name);
v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX;
hash_add(&dp->dv_hashtab, DI2HIKEY(v));
v->di_tv.v_type = VAR_NUMBER;
*** ../vim-7.2.040/src/version.c Wed Nov 12 16:04:43 2008
--- src/version.c Wed Nov 12 16:54:35 2008
***************
*** 678,679 ****
--- 678,681 ----
{ /* Add new patch number below this line */
+ /**/
+ 41,
/**/
--
hundred-and-one symptoms of being an internet addict:
257. Your "hundred-and-one" lists include well over 101 items, since you
automatically interpret all numbers in hexadecimal notation.
(hex 101 = decimal 257)
/// Bram Moolenaar -- [EMAIL PROTECTED] -- 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 ///
--~--~---------~--~----~------------~-------~--~----~
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php
-~----------~----~----~----~------~----~------~--~---
