Consider not overriding _FORTIFY_SOURCE= define: port to c99 flexible arrays

[trofi]

Describe the bug
vim is not _FORTIFY_SOURCE=2 clean (uses char[1] array). This causes the following problems:

• [minor] configure.ac occasionally fails to catch new way to define _FORTIFY_SOURCE=2 and users get cryptic buffer overflow crashes
• [major] gcc assumes that array are 1-byte long and can produce invalid code (similar to what glibc does on _FORTIFY_SOURCE=2 by introspecting array length)

To Reproduce
It's a downstream version of https://bugs.gentoo.org/706324 where gcc-10 was not detected and _FORTIFY_SOURCE=2 default was missed (worked around in #5580).

Expected behavior
vim should build and run on _FORTIFY_SOURCE=2 compiler.

Environment (please complete the following information):

• Vim version: 8.2.0114
• OS: Gentoo
• Terminal: alactitty-0.4.1

If vim can afford using flexible array members (https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html) it might use those:

--- a/src/structs.h
+++ b/src/structs.h
@@ -1414,7 +1414,7 @@ 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!)
+    char_u	di_key[];	// key (actually longer!)
 };
 typedef struct dictitem_S dictitem_T;

I did not check if the rest of code does not rely on sizeof(struct dictitem_S).

[k-takata]

It was reported that some kind of old compilers don't support omitting the array size.

vim/runtime/doc/develop.txt

Line 226 in eed3571

- flexible array members: Not supported by HP-UX C compiler (John Marriott)

Some kind of #ifdef might be needed?

I did not check if the rest of code does not rely on sizeof(struct dictitem_S).

I think that there are many codes that rely on it. Such codes need to be changed.

[k-takata]

Some kind of #ifdef might be needed?

E.g.

#ifdef HAS_FLEXARRY_SUPPORT
# define FLEXARRY_SIZE
# define FLEXARRY_ADD	1
#else
# define FLEXARRY_SIZE	1
# define FLEXARRY_ADD	0
#endif

...

struct dictitem_S
{
    typval_T	di_tv;		// type and value of the variable
    char_u	di_flags;	// DI_FLAGS_ flags (only used for variable)
    char_u	di_key[FLEXARRY_SIZE];	// key (actually longer!)
};

...

	di = alloc(sizeof(dictitem_T) + STRLEN(varname) + FLEXARRY_ADD);

[dpelle]

It was reported that some kind of old compilers don't support omitting the array size.

Some kind of #ifdef might be needed?

Indeed, some compilers unfortunately don't support flexible arrays.
In any case, the main root cause of the issue (incorrect detection of gcc version,
causing to not compile with -D_FORTIFY_SOURCE=1) is fixed in this other PR: #5580

[chrisbra]

This has been discussed before (and actually included as of 561f8a5 and f3a4117). Unfortunately not all compilers support this. See e.g. this thread on patch 8.0.1735 (https://groups.google.com/d/msg/vim_dev/jWEXfiRJv_4/nKEYbB_7CAAJ) when this was rolled back last time (commit 285e335).

I am closing this and leave it to Bram to include the fix in #5580

[trofi]

I'd like to clarify my main worry: gcc already assumes that arrays are 1 byte long and optimises code based on that. -D_FORTIFY_SOURCE=2 is only a side-effect.

If there are no plans to eliminate 1-byte arrays usage would it be feasible/welcome to add runtime check into configure.ac?

The check would fail at ./configure phase with a clear error about unsupported compiler instead of cryptic buffer overflow crash which takes a while to track back to intended use of 1-byte arrays.

[brammool]

I'd like to clarify my main worry: gcc already assumes that arrays are 1 byte long and optimises code based on that. `-D_FORTIFY_SOURCE=2` is only a side-effect. If there are no plans to eliminate 1-byte arrays usage would it be feasible/welcome to add runtime check into `configure.ac`? The check would fail at `./configure` phase with a clear error about unsupported compiler instead of cryptic buffer overflow crash which takes a while to track back to intended use of 1-byte arrays.

This was discussed before: There are still a few compilers that don't support the "empty array" feature. Making two implementations is going to cause duplication of code, where one version won't be used much and can easily break. Best is to just keep it as-is.

…

-- hundred-and-one symptoms of being an internet addict: 36. You miss more than five meals a week downloading the latest games from Apogee. /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[tonymec]

@trofi What's wrong with -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 which has been on the gcc command-line for every Unix source module of Vim since longer than I can remember? If _FORTIFY_SOURCE=2 gives errors and _FORTIFY_SOURCE=1 gives (with the current source) correct code with no errors, then of course we want it to be 1.

[dpelle]

@tonymec wrote:

What's wrong with -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
which has been on the gcc command-line for every Unix source module
of Vim since longer than I can remember?

In my opinion, the problem comes from Ubuntu's silly decision to
patch gcc to enable -D_FORTIFY_SOURCE=2 by default.
That is as far as I know not done by gcc by default and not done
on other distributions (not 100% sure on this last point). On Ubuntu,
doing man gcc shows this:

NOTE: In Ubuntu 8.10 and later versions, -D_FORTIFY_SOURCE=2 is set
by default, and is activated when -O is set to 2 or higher.  This
enables additional compile-time and run-time checks for several
libc functions.  To disable, specify either -U_FORTIFY_SOURCE or
-D_FORTIFY_SOURCE=0.

I think that Ubuntu should not do that by default. -D_FORTIFY_SOURCE=2
is known to break valid C programs whereas -D_FORTIFY_SOURCE=1
should only break programs that have bugs. A project should be
encouraged to use -D_FORTIFY_SOURCE=2, but it should not
be the default compiler setting when not specifying -D_FORTIFY_SOURCE.
Setting a default -D_FORTIFY_SOURCE=1 would have be acceptable.

See the explanation about _FORTIFY_SOURCE at
http://man7.org/linux/man-pages/man7/feature_test_macros.7.html :

If _FORTIFY_SOURCE is set to 1, with compiler optimization
level 1 (gcc -O1) and above, checks that shouldn't change the
behavior of conforming programs are performed.  With
_FORTIFY_SOURCE set to 2, some more checking is added,
but some conforming programs might fail.

Using flexible array members would certainly have been nice.
Unfortunately, it's not supported by all compiler despite being part
of C99. So it was deemed better to not use it. The idea of using
pre-processor ifdef to conditionally use flexible array members is
bad, because it means that behavior is different on different platforms,
and some bugs may occur on some rare platforms only as a result.
Probably, those rare platforms don't even support valgrind or asan
so platform specific bug can go unnoticed. It's better to avoid ifdef
in general as it makes testing harder.