[PATCH] libelf: Do not cast pointer to integer in gelf_newphdr

[PATCH] libelf: Do not cast pointer to integer in gelf_newphdr

[Akihiko Odaki]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

unsigned long int is not always capable to have pointer in some cases
(LLP64, for example).

Signed-off-by: Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
---
 libelf/gelf.h         | 4 ++--
 libelf/gelf_newehdr.c | 6 +++---
 libelf/gelf_newphdr.c | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/libelf/gelf.h b/libelf/gelf.h
index 1bc7ee7..0c31c27 100644
--- a/libelf/gelf.h
+++ b/libelf/gelf.h
@@ -166,7 +166,7 @@ extern GElf_Ehdr *gelf_getehdr (Elf *__elf, GElf_Ehdr *__dest);
 extern int gelf_update_ehdr (Elf *__elf, GElf_Ehdr *__src);
 
 /* Create new ELF header if none exists.  */
-extern unsigned long int gelf_newehdr (Elf *__elf, int __class);
+extern void *gelf_newehdr (Elf *__elf, int __class);
 
 /* Get section at OFFSET.  */
 extern Elf_Scn *gelf_offscn (Elf *__elf, GElf_Off __offset);
@@ -184,7 +184,7 @@ extern GElf_Phdr *gelf_getphdr (Elf *__elf, int __ndx, GElf_Phdr *__dst);
 extern int gelf_update_phdr (Elf *__elf, int __ndx, GElf_Phdr *__src);
 
 /* Create new program header with PHNUM entries.  */
-extern unsigned long int gelf_newphdr (Elf *__elf, size_t __phnum);
+extern void *gelf_newphdr (Elf *__elf, size_t __phnum);
 
 /* Get compression header of section if any.  Returns NULL and sets
    elf_errno if the section isn't compressed or an error occurred.  */
diff --git a/libelf/gelf_newehdr.c b/libelf/gelf_newehdr.c
index cfa80e1..2788906 100644
--- a/libelf/gelf_newehdr.c
+++ b/libelf/gelf_newehdr.c
@@ -37,10 +37,10 @@
 #include "libelfP.h"
 
 
-unsigned long int
+void *
 gelf_newehdr (Elf *elf, int class)
 {
   return (class == ELFCLASS32
-	  ? (unsigned long int) INTUSE(elf32_newehdr) (elf)
-	  : (unsigned long int) INTUSE(elf64_newehdr) (elf));
+	  ? (void *) INTUSE(elf32_newehdr) (elf)
+	  : (void *) INTUSE(elf64_newehdr) (elf));
 }
diff --git a/libelf/gelf_newphdr.c b/libelf/gelf_newphdr.c
index 4e95474..84aad78 100644
--- a/libelf/gelf_newphdr.c
+++ b/libelf/gelf_newphdr.c
@@ -37,10 +37,10 @@
 #include "libelfP.h"
 
 
-unsigned long int
+void *
 gelf_newphdr ( Elf *elf, size_t phnum)
 {
   return (elf->class == ELFCLASS32
-	  ? (unsigned long int) INTUSE(elf32_newphdr) (elf, phnum)
-	  : (unsigned long int) INTUSE(elf64_newphdr) (elf, phnum));
+	  ? (void *) INTUSE(elf32_newphdr) (elf, phnum)
+	  : (void *) INTUSE(elf64_newphdr) (elf, phnum));
 }
-- 
2.10.0

Re: [PATCH] libelf: Do not cast pointer to integer in gelf_newphdr

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On Thu, 2016-10-13 at 12:28 +0200, Mark Wielaard wrote:
> On Tue, 2016-10-11 at 23:12 +0900, Akihiko Odaki wrote:
> > unsigned long int is not always capable to have pointer in some cases
> > (LLP64, for example).
> 
> This makes sense, but it does change a public API. One we share with
> other libelf implementations. So I'll like to discuss this first to make
> sure this doesn't break something subtle.

Discussion finally done. I have added a ChangeLog entry and updated the
documentation a bit. Committed as attached to master now.

Thanks,

Mark

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libelf-gelf_newehdr-and-gelf_newehdr-should-return-v.patch --]
[-- Type: text/x-patch, Size: 3931 bytes --]

From 0d0f8450ffc6135c0938308254f378ac79612e75 Mon Sep 17 00:00:00 2001
From: Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
Date: Tue, 11 Oct 2016 23:12:11 +0900
Subject: [PATCH] libelf: gelf_newehdr and gelf_newehdr should return void *.

unsigned long int is not always capable to have pointer in some cases
(LLP64, for example). Return a void pointer instead. Other libelf
implementations will also make this change (or already have).
Also update the documentation to state what is created and that NULL
is returned on error (don't document that the returned value is a
pointer to the actual header created).

Signed-off-by: Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libelf/ChangeLog      |  8 ++++++++
 libelf/gelf.h         | 12 ++++++++----
 libelf/gelf_newehdr.c |  6 +++---
 libelf/gelf_newphdr.c |  6 +++---
 4 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 6414128..8539cb5 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,11 @@
+2016-10-11  Akihiko Odaki  <akihiko.odaki.4i@stu.hosei.ac.jp>
+	    Mark Wielaard  <mjw@redhat.com>
+
+	* gelf.h (gelf_newehdr): Change return type to void *.
+	(gelf_newphdr): Likewise.
+	* gelf_newehdr.c (gelf_newehdr): Likewise.
+	* gelf_newphdr.c (gelf_newphdr): Likewise.
+
 2016-10-21  Mark Wielaard  <mjw@redhat.com>
 
 	* elf_getdata.c (__libelf_set_rawdata_wrlock): Sanity check
diff --git a/libelf/gelf.h b/libelf/gelf.h
index 1bc7ee7..0619880 100644
--- a/libelf/gelf.h
+++ b/libelf/gelf.h
@@ -165,8 +165,10 @@ extern GElf_Ehdr *gelf_getehdr (Elf *__elf, GElf_Ehdr *__dest);
 /* Update the ELF header.  */
 extern int gelf_update_ehdr (Elf *__elf, GElf_Ehdr *__src);
 
-/* Create new ELF header if none exists.  */
-extern unsigned long int gelf_newehdr (Elf *__elf, int __class);
+/* Create new ELF header if none exists.  Creates an Elf32_Ehdr if CLASS
+   is ELFCLASS32 or an Elf64_Ehdr if CLASS is ELFCLASS64.  Returns NULL
+   on error.  */
+extern void *gelf_newehdr (Elf *__elf, int __class);
 
 /* Get section at OFFSET.  */
 extern Elf_Scn *gelf_offscn (Elf *__elf, GElf_Off __offset);
@@ -183,8 +185,10 @@ extern GElf_Phdr *gelf_getphdr (Elf *__elf, int __ndx, GElf_Phdr *__dst);
 /* Update the program header.  */
 extern int gelf_update_phdr (Elf *__elf, int __ndx, GElf_Phdr *__src);
 
-/* Create new program header with PHNUM entries.  */
-extern unsigned long int gelf_newphdr (Elf *__elf, size_t __phnum);
+/* Create new program header with PHNUM entries.  Creates either an
+   Elf32_Phdr or an Elf64_Phdr depending on whether the given ELF is
+   ELFCLASS32 or ELFCLASS64.  Returns NULL on error.  */
+extern void *gelf_newphdr (Elf *__elf, size_t __phnum);
 
 /* Get compression header of section if any.  Returns NULL and sets
    elf_errno if the section isn't compressed or an error occurred.  */
diff --git a/libelf/gelf_newehdr.c b/libelf/gelf_newehdr.c
index cfa80e1..2788906 100644
--- a/libelf/gelf_newehdr.c
+++ b/libelf/gelf_newehdr.c
@@ -37,10 +37,10 @@
 #include "libelfP.h"
 
 
-unsigned long int
+void *
 gelf_newehdr (Elf *elf, int class)
 {
   return (class == ELFCLASS32
-	  ? (unsigned long int) INTUSE(elf32_newehdr) (elf)
-	  : (unsigned long int) INTUSE(elf64_newehdr) (elf));
+	  ? (void *) INTUSE(elf32_newehdr) (elf)
+	  : (void *) INTUSE(elf64_newehdr) (elf));
 }
diff --git a/libelf/gelf_newphdr.c b/libelf/gelf_newphdr.c
index 4e95474..84aad78 100644
--- a/libelf/gelf_newphdr.c
+++ b/libelf/gelf_newphdr.c
@@ -37,10 +37,10 @@
 #include "libelfP.h"
 
 
-unsigned long int
+void *
 gelf_newphdr ( Elf *elf, size_t phnum)
 {
   return (elf->class == ELFCLASS32
-	  ? (unsigned long int) INTUSE(elf32_newphdr) (elf, phnum)
-	  : (unsigned long int) INTUSE(elf64_newphdr) (elf, phnum));
+	  ? (void *) INTUSE(elf32_newphdr) (elf, phnum)
+	  : (void *) INTUSE(elf64_newphdr) (elf, phnum));
 }
-- 
1.8.3.1

Re: [PATCH] libelf: Do not cast pointer to integer in gelf_newphdr

[Josh Stone]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On 10/13/2016 03:28 AM, Mark Wielaard wrote:
> On Tue, 2016-10-11 at 23:12 +0900, Akihiko Odaki wrote:
>> unsigned long int is not always capable to have pointer in some cases
>> (LLP64, for example).
> 
> This makes sense, but it does change a public API. One we share with
> other libelf implementations. So I'll like to discuss this first to make
> sure this doesn't break something subtle. I believe it is binary
> compatible on any currently supported architecture. And indeed it is
> broken for any architecture that doesn't have sizeof(long) ==
> sizeof(void *). There might be some source compatibility issues. If a
> user stored the result in a long without cast that would now generate a
> warning I suppose.

You could stay integral with uintptr_t -- stdint.h is already included.

Re: [PATCH] libelf: Do not cast pointer to integer in gelf_newphdr

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On Tue, 2016-10-11 at 23:12 +0900, Akihiko Odaki wrote:
> unsigned long int is not always capable to have pointer in some cases
> (LLP64, for example).

This makes sense, but it does change a public API. One we share with
other libelf implementations. So I'll like to discuss this first to make
sure this doesn't break something subtle. I believe it is binary
compatible on any currently supported architecture. And indeed it is
broken for any architecture that doesn't have sizeof(long) ==
sizeof(void *). There might be some source compatibility issues. If a
user stored the result in a long without cast that would now generate a
warning I suppose.

Cheers,

Mark