* Re: [PATCH] Do without on-stack variable length arrays.
@ 2015-09-04 20:51 Roland McGrath
0 siblings, 0 replies; 5+ messages in thread
From: Roland McGrath @ 2015-09-04 20:51 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 335 bytes --]
This is misstated. VLAs are a standard feature. What you're avoiding here
is VLA members of structs and unions, which are a GNU extension.
I also don't think you're doing it the ideal way. A union of two arrays of
length one is useless. It might trigger complaints from _FORTIFY_SOURCE
or other such bounds-checking tools.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Do without on-stack variable length arrays.
@ 2015-09-08 21:10 Chih-Hung Hsieh
0 siblings, 0 replies; 5+ messages in thread
From: Chih-Hung Hsieh @ 2015-09-08 21:10 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
I have replaced this patch with a new one that does not use one element
arrays.
Please take a look of
[PATCH] Do without union of variable length arrays.
Thanks.
On Fri, Sep 4, 2015 at 3:14 PM, Roland McGrath <roland@hack.frob.com> wrote:
> > Is there some simple way that I can test elfutils with fortify or bound
> > checking tools?
>
> Depending on your system and build setup, _FORTIFY_SOURCE might be the
> default. It's easy enough to enable it with CPPFLAGS=-D_FORTIFY_SOURCE=2
> to configure AFAIK.
>
> > How about using pointers and malloc or alloca, and indexing through the
> > pointers? That should avoid complaints from stronger bound checkers,
> > although static bound checking probably won't find bound error either.
>
> Right, that's what I think it will have to be. The cleanest way to do the
> 32/64 variant switching is not immediately clear to me in the abstract,
> though in each particular instance you'll probably be able to decide easily
> enough what feels clean.
>
> > Or is there other pattern preferred by elfutils?
>
> I don't think we've figured anything out.
>
[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1606 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Do without on-stack variable length arrays.
@ 2015-09-04 22:14 Roland McGrath
0 siblings, 0 replies; 5+ messages in thread
From: Roland McGrath @ 2015-09-04 22:14 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 840 bytes --]
> Is there some simple way that I can test elfutils with fortify or bound
> checking tools?
Depending on your system and build setup, _FORTIFY_SOURCE might be the
default. It's easy enough to enable it with CPPFLAGS=-D_FORTIFY_SOURCE=2
to configure AFAIK.
> How about using pointers and malloc or alloca, and indexing through the
> pointers? That should avoid complaints from stronger bound checkers,
> although static bound checking probably won't find bound error either.
Right, that's what I think it will have to be. The cleanest way to do the
32/64 variant switching is not immediately clear to me in the abstract,
though in each particular instance you'll probably be able to decide easily
enough what feels clean.
> Or is there other pattern preferred by elfutils?
I don't think we've figured anything out.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Do without on-stack variable length arrays.
@ 2015-09-04 21:43 Chih-Hung Hsieh
0 siblings, 0 replies; 5+ messages in thread
From: Chih-Hung Hsieh @ 2015-09-04 21:43 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 833 bytes --]
Yes, I shall correct the description.
Is there some simple way that I can test elfutils with fortify or bound
checking tools?
How about using pointers and malloc or alloca, and indexing through the
pointers? That should avoid complaints from stronger bound checkers,
although static bound checking probably won't find bound error either.
Or is there other pattern preferred by elfutils?
On Fri, Sep 4, 2015 at 1:51 PM, Roland McGrath <roland@hack.frob.com> wrote:
> This is misstated. VLAs are a standard feature. What you're avoiding here
> is VLA members of structs and unions, which are a GNU extension.
>
> I also don't think you're doing it the ideal way. A union of two arrays of
> length one is useless. It might trigger complaints from _FORTIFY_SOURCE
> or other such bounds-checking tools.
>
[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1234 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Do without on-stack variable length arrays.
@ 2015-09-04 20:23 Chih-Hung Hsieh
0 siblings, 0 replies; 5+ messages in thread
From: Chih-Hung Hsieh @ 2015-09-04 20:23 UTC (permalink / raw)
To: elfutils-devel
[-- Attachment #1: Type: text/plain, Size: 13166 bytes --]
Prepare to compile without gnu99 extension.
Signed-off-by: Chih-Hung Hsieh <chh@google.com>
---
libdwfl/dwfl_module_getdwarf.c | 16 ++++++++--------
libdwfl/dwfl_segment_report_module.c | 16 ++++++++--------
libdwfl/elf-from-memory.c | 8 ++++----
libdwfl/link_map.c | 26 +++++++++++++-------------
libelf/elf_getarsym.c | 4 ++--
src/readelf.c | 27 +++++++++++++++------------
src/unstrip.c | 20 +++++++++++---------
7 files changed, 61 insertions(+), 56 deletions(-)
diff --git a/libdwfl/dwfl_module_getdwarf.c b/libdwfl/dwfl_module_getdwarf.c
index dba9d66..6d030f0 100644
--- a/libdwfl/dwfl_module_getdwarf.c
+++ b/libdwfl/dwfl_module_getdwarf.c
@@ -370,14 +370,14 @@ find_prelink_address_sync (Dwfl_Module *mod, struct dwfl_file *file)
{
typedef union
{
- Elf32_Phdr p32[phnum];
- Elf64_Phdr p64[phnum];
+ Elf32_Phdr p32[1]; /* phnum elements */
+ Elf64_Phdr p64[1]; /* phnum elements */
} phdr;
- phdr *phdrs = malloc (sizeof (phdr));
+ phdr *phdrs = malloc (sizeof (phdr) * phnum);
if (unlikely (phdrs == NULL))
return DWFL_E_NOMEM;
dst.d_buf = phdrs;
- dst.d_size = sizeof (phdr);
+ dst.d_size = sizeof (phdr) * phnum;
if (unlikely (gelf_xlatetom (mod->main.elf, &dst, &src,
ehdr.e32.e_ident[EI_DATA]) == NULL))
{
@@ -414,14 +414,14 @@ find_prelink_address_sync (Dwfl_Module *mod, struct dwfl_file *file)
typedef union
{
- Elf32_Shdr s32[shnum - 1];
- Elf64_Shdr s64[shnum - 1];
+ Elf32_Shdr s32[1]; /* (shnum - 1) elements */
+ Elf64_Shdr s64[1]; /* (shnum - 1) elements */
} shdr;
- shdr *shdrs = malloc (sizeof (shdr));
+ shdr *shdrs = malloc (sizeof (shdr) * (shnum - 1));
if (unlikely (shdrs == NULL))
return DWFL_E_NOMEM;
dst.d_buf = shdrs;
- dst.d_size = sizeof (shdr);
+ dst.d_size = sizeof (shdr) * (shnum - 1);
if (unlikely (gelf_xlatetom (mod->main.elf, &dst, &src,
ehdr.e32.e_ident[EI_DATA]) == NULL))
{
diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c
index a0f07ad..8667538 100644
--- a/libdwfl/dwfl_segment_report_module.c
+++ b/libdwfl/dwfl_segment_report_module.c
@@ -406,17 +406,17 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
typedef union
{
- Elf32_Phdr p32[phnum];
- Elf64_Phdr p64[phnum];
+ Elf32_Phdr p32[1]; /* phnum elements */
+ Elf64_Phdr p64[1]; /* phnum elements */
} phdrsn;
- phdrsp = malloc (sizeof (phdrsn));
+ phdrsp = malloc (sizeof (phdrsn) * phnum);
if (unlikely (phdrsp == NULL))
return finish ();
phdrsn *phdrs = (phdrsn *) phdrsp;
xlateto.d_buf = phdrs;
- xlateto.d_size = sizeof (phdrsn);
+ xlateto.d_size = sizeof (phdrsn) * phnum;
/* Track the bounds of the file visible in memory. */
GElf_Off file_trimmed_end = 0; /* Proper p_vaddr + p_filesz end. */
@@ -757,10 +757,10 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
{
typedef union
{
- Elf32_Dyn d32[dyn_filesz / sizeof (Elf32_Dyn)];
- Elf64_Dyn d64[dyn_filesz / sizeof (Elf64_Dyn)];
+ Elf32_Dyn d32[1]; /* dyn_filesz / sizeof (Elf32_Dyn) */
+ Elf64_Dyn d64[1]; /* dyn_filesz / sizeof (Elf64_Dyn) */
} dynn;
- dyns = malloc (sizeof (dynn));
+ dyns = malloc (dyn_filesz);
if (unlikely (dyns == NULL))
return finish ();
dynn *dyn = (dynn *) dyns;
@@ -769,7 +769,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
xlatefrom.d_buf = (void *) dyn_data;
xlatefrom.d_size = dyn_filesz;
xlateto.d_buf = dyn;
- xlateto.d_size = sizeof (dynn);
+ xlateto.d_size = dyn_filesz;
if (ei_class == ELFCLASS32)
{
diff --git a/libdwfl/elf-from-memory.c b/libdwfl/elf-from-memory.c
index ed8f6e9..dc23845 100644
--- a/libdwfl/elf-from-memory.c
+++ b/libdwfl/elf-from-memory.c
@@ -193,11 +193,11 @@ elf_from_remote_memory (GElf_Addr ehdr_vma,
typedef union
{
- Elf32_Phdr p32[phnum];
- Elf64_Phdr p64[phnum];
+ Elf32_Phdr p32[1]; /* phnum elements */
+ Elf64_Phdr p64[1]; /* phnum elements */
} phdrsn;
- phdrsp = malloc (sizeof (phdrsn));
+ phdrsp = malloc (sizeof (phdrsn) * phnum);
if (unlikely (phdrsp == NULL))
{
free (buffer);
@@ -206,7 +206,7 @@ elf_from_remote_memory (GElf_Addr ehdr_vma,
phdrsn *phdrs = (phdrsn *) phdrsp;
xlateto.d_buf = phdrs;
- xlateto.d_size = sizeof (phdrsn);
+ xlateto.d_size = sizeof (phdrsn) * phnum;
/* Scan for PT_LOAD segments to find the total size of the file image. */
size_t contents_size = 0;
diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
index 030c600..b27c9f1 100644
--- a/libdwfl/link_map.c
+++ b/libdwfl/link_map.c
@@ -50,9 +50,9 @@ auxv_format_probe (const void *auxv, size_t size,
{
const union
{
- char buf[size];
- Elf32_auxv_t a32[size / sizeof (Elf32_auxv_t)];
- Elf64_auxv_t a64[size / sizeof (Elf64_auxv_t)];
+ char buf[1]; /* size bytes */
+ Elf32_auxv_t a32[1];
+ Elf64_auxv_t a64[1];
} *u = auxv;
inline bool check64 (size_t i)
@@ -282,8 +282,8 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
const union
{
- Elf32_Addr a32[n];
- Elf64_Addr a64[n];
+ Elf32_Addr a32[1]; /* n elements */
+ Elf64_Addr a64[1]; /* n elements */
} *in = vaddr - read_vaddr + buffer;
if (elfclass == ELFCLASS32)
@@ -865,9 +865,9 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
{
Elf32_Phdr p32;
Elf64_Phdr p64;
- char data[phnum * phent];
+ char data[1]; /* (phnum * phent) bytes */
} data_buf;
- data_buf *buf = malloc (sizeof (data_buf));
+ data_buf *buf = malloc (phnum * phent);
if (unlikely (buf == NULL))
{
__libdwfl_seterrno (DWFL_E_NOMEM);
@@ -888,8 +888,8 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
/* We are looking for PT_DYNAMIC. */
const union
{
- Elf32_Phdr p32[phnum];
- Elf64_Phdr p64[phnum];
+ Elf32_Phdr p32[1]; /* phnum elements */
+ Elf64_Phdr p64[1]; /* phnum elements */
} *u = (void *) buf;
if (elfclass == ELFCLASS32)
{
@@ -959,9 +959,9 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
{
Elf32_Dyn d32;
Elf64_Dyn d64;
- char data[dyn_filesz];
+ char data[1]; /* dyn_filesz bytes */
} data_buf;
- data_buf *buf = malloc (sizeof (data_buf));
+ data_buf *buf = malloc (dyn_filesz);
if (unlikely (buf == NULL))
{
__libdwfl_seterrno (DWFL_E_NOMEM);
@@ -982,8 +982,8 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
/* We are looking for DT_DEBUG. */
const union
{
- Elf32_Dyn d32[dyn_filesz / sizeof (Elf32_Dyn)];
- Elf64_Dyn d64[dyn_filesz / sizeof (Elf64_Dyn)];
+ Elf32_Dyn d32[1]; /* dyn_filesz bytes */
+ Elf64_Dyn d64[1]; /* dyn_filesz bytes */
} *u = (void *) buf;
if (elfclass == ELFCLASS32)
{
diff --git a/libelf/elf_getarsym.c b/libelf/elf_getarsym.c
index 8324244..569477e 100644
--- a/libelf/elf_getarsym.c
+++ b/libelf/elf_getarsym.c
@@ -205,8 +205,8 @@ elf_getarsym (elf, ptr)
{
union
{
- uint32_t u32[n];
- uint64_t u64[n];
+ uint32_t u32[1]; /* n elements */
+ uint64_t u64[1]; /* n elements */
} *file_data;
char *str_data;
size_t sz = n * w;
diff --git a/src/readelf.c b/src/readelf.c
index d3c2b6b..ef66f0d 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -4943,7 +4943,7 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
unsigned int version, unsigned int ptr_size,
Dwfl_Module *dwflmod, Ebl *ebl, Dwarf *dbg)
{
- char regnamebuf[REGNAMESZ];
+ char *regnamebuf = alloca (REGNAMESZ);
const char *regname (unsigned int regno)
{
register_info (ebl, regno, NULL, regnamebuf, NULL, NULL);
@@ -8378,11 +8378,13 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
DO_TYPE (XWORD, Xword, "0x%.16" PRIx64, "%" PRId64); \
DO_TYPE (SXWORD, Sxword, "%" PRId64, "%" PRId64)
-#define DO_TYPE(NAME, Name, hex, dec) GElf_##Name Name[count]
- union { TYPES; } value;
+#define DO_TYPE(NAME, Name, hex, dec) GElf_##Name Name[2]
+ /* Name should have count elements and count >= 2 */
+ typedef union { TYPES; } value_t;
+ value_t *value = alloca (sizeof (value_t) / 2 * count);
#undef DO_TYPE
- void *data = &value;
+ void *data = value;
size_t size = gelf_fsize (core, item->type, count, EV_CURRENT);
size_t convsize = size;
if (repeated_size != NULL)
@@ -8413,7 +8415,7 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
#define DO_TYPE(NAME, Name, hex, dec) \
case ELF_T_##NAME: \
colno = print_core_item (colno, ',', WRAP_COLUMN, \
- 0, item->name, dec, value.Name[0]); \
+ 0, item->name, dec, value->Name[0]); \
break
TYPES;
#undef DO_TYPE
@@ -8429,7 +8431,7 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
#define DO_TYPE(NAME, Name, hex, dec) \
case ELF_T_##NAME: \
colno = print_core_item (colno, ',', WRAP_COLUMN, \
- 0, item->name, hex, value.Name[0]); \
+ 0, item->name, hex, value->Name[0]); \
break
TYPES;
#undef DO_TYPE
@@ -8511,8 +8513,8 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
{
#define DO_TYPE(NAME, Name, hex, dec) \
case ELF_T_##NAME: \
- sec = value.Name[0]; \
- usec = value.Name[1]; \
+ sec = value->Name[0]; \
+ usec = value->Name[1]; \
break
TYPES;
#undef DO_TYPE
@@ -8542,12 +8544,12 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
case 'c':
assert (count == 1);
colno = print_core_item (colno, ',', WRAP_COLUMN, 0, item->name,
- "%c", value.Byte[0]);
+ "%c", value->Byte[0]);
break;
case 's':
colno = print_core_item (colno, ',', WRAP_COLUMN, 0, item->name,
- "%.*s", (int) count, value.Byte);
+ "%.*s", (int) count, value->Byte);
break;
case '\n':
@@ -8891,8 +8893,9 @@ handle_core_registers (Ebl *ebl, Elf *core, const void *desc,
assert (maxnreg > 0);
}
- struct register_info regs[maxnreg];
- memset (regs, 0, sizeof regs);
+ const int sizeof_regs = sizeof (struct register_info) * maxnreg;
+ struct register_info *regs = alloca (sizeof_regs);
+ memset (regs, 0, sizeof_regs);
/* Sort to collect the sets together. */
int maxreg = 0;
diff --git a/src/unstrip.c b/src/unstrip.c
index 8833094..3f25201 100644
--- a/src/unstrip.c
+++ b/src/unstrip.c
@@ -1013,13 +1013,14 @@ find_alloc_sections_prelink (Elf *debug, Elf_Data *debug_shstrtab,
error (EXIT_FAILURE, 0, _("invalid contents in '%s' section"),
".gnu.prelink_undo");
- union
+ typedef union
{
- Elf32_Shdr s32[shnum - 1];
- Elf64_Shdr s64[shnum - 1];
- } shdr;
- dst.d_buf = &shdr;
- dst.d_size = sizeof shdr;
+ Elf32_Shdr s32[1]; /* (shnum - 1) elements */
+ Elf64_Shdr s64[1]; /* (shnum - 1) elements */
+ } shdr_t;
+ shdr_t *shdr = alloca (sizeof (shdr_t) * (shnum - 1));
+ dst.d_buf = shdr;
+ dst.d_size = sizeof (shdr_t) * (shnum - 1);
ELF_CHECK (gelf_xlatetom (main, &dst, &src,
main_ehdr->e_ident[EI_DATA]) != NULL,
_("cannot read '.gnu.prelink_undo' section: %s"));
@@ -1030,7 +1031,7 @@ find_alloc_sections_prelink (Elf *debug, Elf_Data *debug_shstrtab,
struct section *sec = &undo_sections[undo_nalloc];
if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32)
{
-#define COPY(field) sec->shdr.field = shdr.s32[i].field
+#define COPY(field) sec->shdr.field = shdr->s32[i].field
COPY (sh_name);
COPY (sh_type);
COPY (sh_flags);
@@ -1044,7 +1045,7 @@ find_alloc_sections_prelink (Elf *debug, Elf_Data *debug_shstrtab,
#undef COPY
}
else
- sec->shdr = shdr.s64[i];
+ sec->shdr = shdr->s64[i];
if (sec->shdr.sh_flags & SHF_ALLOC)
{
sec->shdr.sh_addr += bias;
@@ -1260,7 +1261,8 @@ copy_elided_sections (Elf *unstripped, Elf *stripped,
more sections in stripped file than debug file -- arguments reversed?"));
/* Cache the stripped file's section details. */
- struct section sections[stripped_shnum - 1];
+ struct section *sections =
+ alloca (sizeof (struct section) * (stripped_shnum - 1));
Elf_Scn *scn = NULL;
while ((scn = elf_nextscn (stripped, scn)) != NULL)
{
--
2.5.0.457.gab17608
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-09-08 21:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-04 20:51 [PATCH] Do without on-stack variable length arrays Roland McGrath
-- strict thread matches above, loose matches on Subject: below --
2015-09-08 21:10 Chih-Hung Hsieh
2015-09-04 22:14 Roland McGrath
2015-09-04 21:43 Chih-Hung Hsieh
2015-09-04 20:23 Chih-Hung Hsieh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).