| Message ID | 20210127085752.120571-4-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with -Wextra | expand |
On 27/01/2021 09.57, Alexey Kardashevskiy wrote: > -Wextra enables a bunch of rather useful checks which this fixes. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- [...] > diff --git a/lib/libelf/elf32.c b/lib/libelf/elf32.c > index 64ea386a96da..b213a6f9ca17 100644 > --- a/lib/libelf/elf32.c > +++ b/lib/libelf/elf32.c > @@ -212,13 +212,13 @@ elf_byteswap_header32(void *file_addr) > * file. > * @return Return -1 on error, size of file otherwise. > */ > -long elf_get_file_size32(const void *buffer, const long buffer_size) > +long elf_get_file_size32(const void *buffer, const unsigned long buffer_size) > { > const struct ehdr32 *ehdr = (const struct ehdr32 *) buffer; > const uint8_t *buffer_end = buffer + buffer_size; > const struct phdr32 *phdr; > const struct shdr32 *shdr; > - long elf_size = -1; > + unsigned long elf_size = -1; That looks kind of ugly, since the return type of the function is signed again later... Maybe it's better to cast in the MAX() function instead? > diff --git a/lib/libelf/elf64.c b/lib/libelf/elf64.c > index 0f302679f784..eb4cc3a7eab3 100644 > --- a/lib/libelf/elf64.c > +++ b/lib/libelf/elf64.c > @@ -389,7 +389,7 @@ elf_apply_all_rela64(void *file_addr, signed long offset, struct shdr64 *shdrs, > struct rela *relaentry; > struct sym64 *symtabentry; > uint32_t symbolidx; > - int i; > + unsigned i; > > /* If the referenced section has not been allocated, then it has > * not been loaded and thus does not need to be relocated. */ > @@ -481,13 +481,13 @@ uint32_t elf_get_eflags_64(void *file_addr) > * file. > * @return Return -1 on error, size of file otherwise. > */ > -long elf_get_file_size64(const void *buffer, const long buffer_size) > +long elf_get_file_size64(const void *buffer, const unsigned long buffer_size) > { > const struct ehdr64 *ehdr = (const struct ehdr64 *) buffer; > const uint8_t *buffer_end = buffer + buffer_size; > const struct phdr64 *phdr; > const struct shdr64 *shdr; > - long elf_size = -1; > + unsigned elf_size = (unsigned)-1; Should that be unsigned *long* instead ? Thomas
On 28/01/2021 02:18, Thomas Huth wrote: > On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >> -Wextra enables a bunch of rather useful checks which this fixes. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- > [...] >> diff --git a/lib/libelf/elf32.c b/lib/libelf/elf32.c >> index 64ea386a96da..b213a6f9ca17 100644 >> --- a/lib/libelf/elf32.c >> +++ b/lib/libelf/elf32.c >> @@ -212,13 +212,13 @@ elf_byteswap_header32(void *file_addr) >> * file. >> * @return Return -1 on error, size of file otherwise. >> */ >> -long elf_get_file_size32(const void *buffer, const long buffer_size) >> +long elf_get_file_size32(const void *buffer, const unsigned long >> buffer_size) >> { >> const struct ehdr32 *ehdr = (const struct ehdr32 *) buffer; >> const uint8_t *buffer_end = buffer + buffer_size; >> const struct phdr32 *phdr; >> const struct shdr32 *shdr; >> - long elf_size = -1; >> + unsigned long elf_size = -1; > > That looks kind of ugly, since the return type of the function is signed > again later... True. > Maybe it's better to cast in the MAX() function instead? gcc happily casts the result of "+" to a signed type. It is easier to do this before returning so I'll post it soon: if (elf_size > 0 && (unsigned long) elf_size > buffer_size) return -1; > >> diff --git a/lib/libelf/elf64.c b/lib/libelf/elf64.c >> index 0f302679f784..eb4cc3a7eab3 100644 >> --- a/lib/libelf/elf64.c >> +++ b/lib/libelf/elf64.c >> @@ -389,7 +389,7 @@ elf_apply_all_rela64(void *file_addr, signed long >> offset, struct shdr64 *shdrs, >> struct rela *relaentry; >> struct sym64 *symtabentry; >> uint32_t symbolidx; >> - int i; >> + unsigned i; >> /* If the referenced section has not been allocated, then it has >> * not been loaded and thus does not need to be relocated. */ >> @@ -481,13 +481,13 @@ uint32_t elf_get_eflags_64(void *file_addr) >> * file. >> * @return Return -1 on error, size of file otherwise. >> */ >> -long elf_get_file_size64(const void *buffer, const long buffer_size) >> +long elf_get_file_size64(const void *buffer, const unsigned long >> buffer_size) >> { >> const struct ehdr64 *ehdr = (const struct ehdr64 *) buffer; >> const uint8_t *buffer_end = buffer + buffer_size; >> const struct phdr64 *phdr; >> const struct shdr64 *shdr; >> - long elf_size = -1; >> + unsigned elf_size = (unsigned)-1; > > Should that be unsigned *long* instead ? Ouch, missed this one, thanks for spotting it. > > Thomas >
On 28/01/2021 14:41, Alexey Kardashevskiy wrote: > > > On 28/01/2021 02:18, Thomas Huth wrote: >> On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >>> -Wextra enables a bunch of rather useful checks which this fixes. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >> [...] >>> diff --git a/lib/libelf/elf32.c b/lib/libelf/elf32.c >>> index 64ea386a96da..b213a6f9ca17 100644 >>> --- a/lib/libelf/elf32.c >>> +++ b/lib/libelf/elf32.c >>> @@ -212,13 +212,13 @@ elf_byteswap_header32(void *file_addr) >>> * file. >>> * @return Return -1 on error, size of file otherwise. >>> */ >>> -long elf_get_file_size32(const void *buffer, const long buffer_size) >>> +long elf_get_file_size32(const void *buffer, const unsigned long >>> buffer_size) >>> { >>> const struct ehdr32 *ehdr = (const struct ehdr32 *) buffer; >>> const uint8_t *buffer_end = buffer + buffer_size; >>> const struct phdr32 *phdr; >>> const struct shdr32 *shdr; >>> - long elf_size = -1; >>> + unsigned long elf_size = -1; >> >> That looks kind of ugly, since the return type of the function is >> signed again later... > > True. > >> Maybe it's better to cast in the MAX() function instead? > > gcc happily casts the result of "+" to a signed type. ouch, no, never mind, there are actually more warnings to fix.
diff --git a/include/libelf.h b/include/libelf.h index 48ff4d7beb68..29a4d049a096 100644 --- a/include/libelf.h +++ b/include/libelf.h @@ -96,9 +96,9 @@ void elf_relocate64(void *file_addr, signed long offset); int elf_forth_claim(void *addr, long size); -long elf_get_file_size(const void *buffer, const long buffer_size); -long elf_get_file_size32(const void *buffer, const long buffer_size); -long elf_get_file_size64(const void *buffer, const long buffer_size); +long elf_get_file_size(const void *buffer, const unsigned long buffer_size); +long elf_get_file_size32(const void *buffer, const unsigned long buffer_size); +long elf_get_file_size64(const void *buffer, const unsigned long buffer_size); #ifdef __BIG_ENDIAN__ #define elf64_to_cpu(x, ehdr) ((ehdr)->ei_data == ELFDATA2MSB ? (x) : bswap_64(x)) diff --git a/lib/libelf/elf.c b/lib/libelf/elf.c index d3684545aef7..f6a052ef3c9e 100644 --- a/lib/libelf/elf.c +++ b/lib/libelf/elf.c @@ -202,7 +202,7 @@ elf_get_base_addr(void *file_addr) * buffer larger than the size of the file * @return The size of the ELF image or < 0 for error */ -long elf_get_file_size(const void *buffer, const long buffer_size) +long elf_get_file_size(const void *buffer, const unsigned long buffer_size) { const struct ehdr *ehdr = (const struct ehdr *)buffer; diff --git a/lib/libelf/elf32.c b/lib/libelf/elf32.c index 64ea386a96da..b213a6f9ca17 100644 --- a/lib/libelf/elf32.c +++ b/lib/libelf/elf32.c @@ -212,13 +212,13 @@ elf_byteswap_header32(void *file_addr) * file. * @return Return -1 on error, size of file otherwise. */ -long elf_get_file_size32(const void *buffer, const long buffer_size) +long elf_get_file_size32(const void *buffer, const unsigned long buffer_size) { const struct ehdr32 *ehdr = (const struct ehdr32 *) buffer; const uint8_t *buffer_end = buffer + buffer_size; const struct phdr32 *phdr; const struct shdr32 *shdr; - long elf_size = -1; + unsigned long elf_size = -1; uint16_t entsize; unsigned i; diff --git a/lib/libelf/elf64.c b/lib/libelf/elf64.c index 0f302679f784..eb4cc3a7eab3 100644 --- a/lib/libelf/elf64.c +++ b/lib/libelf/elf64.c @@ -389,7 +389,7 @@ elf_apply_all_rela64(void *file_addr, signed long offset, struct shdr64 *shdrs, struct rela *relaentry; struct sym64 *symtabentry; uint32_t symbolidx; - int i; + unsigned i; /* If the referenced section has not been allocated, then it has * not been loaded and thus does not need to be relocated. */ @@ -481,13 +481,13 @@ uint32_t elf_get_eflags_64(void *file_addr) * file. * @return Return -1 on error, size of file otherwise. */ -long elf_get_file_size64(const void *buffer, const long buffer_size) +long elf_get_file_size64(const void *buffer, const unsigned long buffer_size) { const struct ehdr64 *ehdr = (const struct ehdr64 *) buffer; const uint8_t *buffer_end = buffer + buffer_size; const struct phdr64 *phdr; const struct shdr64 *shdr; - long elf_size = -1; + unsigned elf_size = (unsigned)-1; uint16_t entsize; unsigned i;
-Wextra enables a bunch of rather useful checks which this fixes. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- include/libelf.h | 6 +++--- lib/libelf/elf.c | 2 +- lib/libelf/elf32.c | 4 ++-- lib/libelf/elf64.c | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-)