[ipxe-devel] [PATCH] [efi] Use memcpy to handle efi header name

[Bernhard M. Wiedemann]

strncpy will copy 8 chars into a buffer of size 8
which does not leave space for the terminating null byte,
thus gcc8 failed compilation with

In function 'process_section',
inlined from 'elf2pe.isra.4' at util/elf2efi.c:913:25:
util/elf2efi.c:497:2: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]

It seems hdr.Name is not supposed to be null-terminated,
so memcpy is more appropriate.

Signed-off-by: Bernhard M. Wiedemann <***@suse.de>
---
src/util/elf2efi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/elf2efi.c b/src/util/elf2efi.c
index 6718df77..de3c9246 100644
--- a/src/util/elf2efi.c
+++ b/src/util/elf2efi.c
@@ -494,7 +494,7 @@ static struct pe_section * process_section ( struct elf_file *elf,
memset ( new, 0, sizeof ( *new ) + section_filesz );

/* Fill in section header details */
- strncpy ( ( char * ) new->hdr.Name, name, sizeof ( new->hdr.Name ) );
+ memcpy ( ( char * ) new->hdr.Name, name, sizeof ( new->hdr.Name ) );
new->hdr.Misc.VirtualSize = section_memsz;
new->hdr.VirtualAddress = shdr->sh_addr;
new->hdr.SizeOfRawData = section_filesz;

[Christian Hesse]

Post by Bernhard M. Wiedemann
strncpy will copy 8 chars into a buffer of size 8
which does not leave space for the terminating null byte,
thus gcc8 failed compilation with
In function 'process_section',
util/elf2efi.c:497:2: error: 'strncpy' specified bound 8 equals
destination size [-Werror=stringop-truncation]
It seems hdr.Name is not supposed to be null-terminated,
so memcpy is more appropriate.

I send an identical patch (except the message of course) on May 15th. And
there were different solutions on the mailing list as well.

Michael, can we commit a fix for this, please?

[Christian Nilsson]

Post by Christian Hesse

Post by Bernhard M. Wiedemann
strncpy will copy 8 chars into a buffer of size 8
which does not leave space for the terminating null byte,
thus gcc8 failed compilation with
In function 'process_section',
util/elf2efi.c:497:2: error: 'strncpy' specified bound 8 equals
destination size [-Werror=stringop-truncation]
It seems hdr.Name is not supposed to be null-terminated,
so memcpy is more appropriate.

I send an identical patch (except the message of course) on May 15th. And
there were different solutions on the mailing list as well.
Michael, can we commit a fix for this, please?

This is probably the 3rd thread about this, so let's go back to the
original thread:
http://lists.ipxe.org/pipermail/ipxe-devel/2018-April/006144.html

[Michael Brown]

Post by Christian Nilsson

Post by Christian Hesse
I send an identical patch (except the message of course) on May 15th. And
there were different solutions on the mailing list as well.
Michael, can we commit a fix for this, please?

This is probably the 3rd thread about this, so let's go back to the
http://lists.ipxe.org/pipermail/ipxe-devel/2018-April/006144.html

Yes, I will pick an appropriate patch and push it (with credits to
everyone who has also supplied a fix).

Thank you to everyone who's reported/fixed this.

Michael

[Bernhard M. Wiedemann]

From: Bruce Rogers <***@suse.com>

Using gcc 8 with the -Wstringop-truncation option, and with warnings
treated as errors, the following error is emitted:

util/elf2efi.c:494:2: error: 'strncpy' specified bound 8 equals destination
size [-Werror=stringop-truncation]
strncpy ( ( char * ) new->hdr.Name, name, sizeof ( new->hdr.Name ) );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Use gcc pragmas surrounding this line of code which avoid this warning. The
strncpy usage here is correct since the destination should be null-padded
but not null-terminated.

[BR: BSC#1090355]
Signed-off-by: Bruce Rogers <***@suse.com>
Acked-by: Bernhard M. Wiedemann <***@suse.de>

---
In http://lists.ipxe.org/pipermail/ipxe-devel/2018-April/006144.html

I'll happily accept patches to alter gcc's behaviour so that it does not
report false positive warnings for these 100% correct usages of
strncpy().

So this patch (pulled from openSUSE's qemu package) seems most appropriate.
---
src/util/elf2efi.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/src/util/elf2efi.c b/src/util/elf2efi.c
index 6718df77..93cdda03 100644
--- a/src/util/elf2efi.c
+++ b/src/util/elf2efi.c
@@ -494,7 +494,13 @@ static struct pe_section * process_section ( struct elf_file *elf,
memset ( new, 0, sizeof ( *new ) + section_filesz );

/* Fill in section header details */
+/* gcc 8 warning gives false positive here - our usage is correct */
+#pragma GCC diagnostic push
+#if __GNUC__ >= 8
+#pragma GCC diagnostic ignored "-Wstringop-truncation"
+#endif
strncpy ( ( char * ) new->hdr.Name, name, sizeof ( new->hdr.Name ) );
+#pragma GCC diagnostic pop
new->hdr.Misc.VirtualSize = section_memsz;
new->hdr.VirtualAddress = shdr->sh_addr;
new->hdr.SizeOfRawData = section_filesz;

[Michael Brown]

Post by Michael Brown
Yes, I will pick an appropriate patch and push it (with credits to
everyone who has also supplied a fix).
Thank you to everyone who's reported/fixed this.

I have pushed:

http://git.ipxe.org/ipxe.git/commitdiff/8ed4e3049

I spent some time trying to get __attribute__((nonstring)) to work, as
inspired by Olaf's patch. Unfortunately it seems that casting to a
pointer with __attribute__((nonstring)) doesn't work to silence the
warning, and the only working approach seemed to require Olaf's original
modification to PeImage.h. Since PeImage.h is imported from the EDK2
codebase, patching it isn't a viable option.

The EDK2 codebase has adopted the use of -Wno-stringop-truncation, so it
is unlikely that a future version of PeImage.h would specify
__attribute__((nonstring)) on the relevant field.

Ultimately, using -Wno-stringop-truncation seems to be the cleanest and
most maintainable solution.

Thanks to everyone who contributed, and sorry for the delay.

Michael