[PATCH] gelf_getauxv: Use memcpy instead of pointer dereference to avoid alignment problems.
--- libelf/ChangeLog | 5 +++++ libelf/gelf_getauxv.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/libelf/ChangeLog b/libelf/ChangeLog index 674a720..46d4731 100644 --- a/libelf/ChangeLog +++ b/libelf/ChangeLog @@ -1,3 +1,8 @@ +2013-08-25 Kurt Roeckx kurt@roeckx.be + + * gelf_getauxv.c (gelf_getauxv): Use memcpy instead of pointer + dereference to avoid alignment problems. + 2013-01-07 Roland McGrath roland@hack.frob.com
* elf_getarsym.c (elf_getarsym): Copy FILE_DATA into stack space if it diff --git a/libelf/gelf_getauxv.c b/libelf/gelf_getauxv.c index d87362e..be24667 100644 --- a/libelf/gelf_getauxv.c +++ b/libelf/gelf_getauxv.c @@ -97,7 +97,7 @@ gelf_getauxv (data, ndx, dst) goto out; }
- *dst = ((GElf_auxv_t *) data_scn->d.d_buf)[ndx]; + memcpy(dst, &((GElf_auxv_t *) data_scn->d.d_buf)[ndx], sizeof(*dst)); }
result = dst;
On 08/25/2013 12:47 PM, Kurt Roeckx wrote:
*dst = ((GElf_auxv_t *) data_scn->d.d_buf)[ndx];
memcpy(dst, &((GElf_auxv_t *) data_scn->d.d_buf)[ndx], sizeof(*dst));
That's not correct, the cast is already undefined if the pointer lacks sufficient alignment. GCC might even translate the latter into the former.
This might do it:
memcpy(dst, data_scn->d.d_buf + ndx * sizeof(GElf_auxv_t), sizeof(GElf_axuv_t));
(I hope there's a bounds check on ndx before that. :-)
On Sun, Aug 25, 2013 at 09:18:42PM +0200, Florian Weimer wrote:
On 08/25/2013 12:47 PM, Kurt Roeckx wrote:
*dst = ((GElf_auxv_t *) data_scn->d.d_buf)[ndx];
memcpy(dst, &((GElf_auxv_t *) data_scn->d.d_buf)[ndx], sizeof(*dst));That's not correct, the cast is already undefined if the pointer lacks sufficient alignment. GCC might even translate the latter into the former.
You're of course right that that case is undefined behavior.
memcpy(dst, data_scn->d.d_buf + ndx * sizeof(GElf_auxv_t), sizeof(GElf_axuv_t));
d_buf is a void *, and I don't think that has defined behavior either. It should probably be: memcpy(dst, (char *)data_scn->d.d_buf + ndx * sizeof(GElf_auxv_t), sizeof(GElf_axuv_t));
Kurt
On Sun, Aug 25, 2013 at 09:40:49PM +0200, Kurt Roeckx wrote:
It should probably be: memcpy(dst, (char *)data_scn->d.d_buf + ndx * sizeof(GElf_auxv_t), sizeof(GElf_axuv_t));
I do think so yes. On which architecture did you see the alignment problem? Is it easily repeatable?
BTW. Could you read the CONTRIBUTING file and add a Signed-off-by line if possible to your patch.
Thanks,
Mark
On Sun, Aug 25, 2013 at 11:05:14PM +0200, Mark Wielaard wrote:
On Sun, Aug 25, 2013 at 09:40:49PM +0200, Kurt Roeckx wrote:
It should probably be: memcpy(dst, (char *)data_scn->d.d_buf + ndx * sizeof(GElf_auxv_t), sizeof(GElf_axuv_t));
I do think so yes. On which architecture did you see the alignment problem? Is it easily repeatable?
This is the problem I see on sparc.
BTW. Could you read the CONTRIBUTING file and add a Signed-off-by line if possible to your patch.
I forgot about that. Will send new patch shortly.
Kurt
Signed-off-by: Kurt Roeckx kurt@roeckx.be --- libelf/ChangeLog | 5 +++++ libelf/gelf_getauxv.c | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/libelf/ChangeLog b/libelf/ChangeLog index 674a720..46d4731 100644 --- a/libelf/ChangeLog +++ b/libelf/ChangeLog @@ -1,3 +1,8 @@ +2013-08-25 Kurt Roeckx kurt@roeckx.be + + * gelf_getauxv.c (gelf_getauxv): Use memcpy instead of pointer + dereference to avoid alignment problems. + 2013-01-07 Roland McGrath roland@hack.frob.com
* elf_getarsym.c (elf_getarsym): Copy FILE_DATA into stack space if it diff --git a/libelf/gelf_getauxv.c b/libelf/gelf_getauxv.c index d87362e..65b8ae0 100644 --- a/libelf/gelf_getauxv.c +++ b/libelf/gelf_getauxv.c @@ -97,7 +97,8 @@ gelf_getauxv (data, ndx, dst) goto out; }
- *dst = ((GElf_auxv_t *) data_scn->d.d_buf)[ndx]; + memcpy(dst, (char *)data_scn->d.d_buf + ndx * sizeof(GElf_auxv_t), + sizeof(GElf_auxv_t)); }
result = dst;
On Sun, 2013-08-25 at 23:15 +0200, Kurt Roeckx wrote:
+2013-08-25 Kurt Roeckx kurt@roeckx.be
- gelf_getauxv.c (gelf_getauxv): Use memcpy instead of pointer
- dereference to avoid alignment problems.
Pushed with a small white-space tweak.
Thanks,
Mark
On Sunday 25 August 2013 15:40:49 Kurt Roeckx wrote:
On Sun, Aug 25, 2013 at 09:18:42PM +0200, Florian Weimer wrote:
memcpy(dst, data_scn->d.d_buf + ndx * sizeof(GElf_auxv_t), sizeof(GElf_axuv_t));
d_buf is a void *, and I don't think that has defined behavior either. It should probably be: memcpy(dst, (char *)data_scn->d.d_buf + ndx * sizeof(GElf_auxv_t), sizeof(GElf_axuv_t));
you're right that adding to a void* is undefined. but practically speaking, glibc requires gcc which has known semantics (the same if you cast it to a char*), so i wonder if the cast is necessary. -mike
Arithmetic on void * is a GNU extension and we use it freely in elfutils code. Don't insert unnecessary casts to char *.
On Mon, 2013-08-26 at 15:29 -0700, Roland McGrath wrote:
Arithmetic on void * is a GNU extension and we use it freely in elfutils code. Don't insert unnecessary casts to char *.
I admit I wasn't aware of that extension http://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html Treating the size of void as 1 does seem to be the only logical (and convenient) choice. Now that I have seen it I wonder why the standard doesn't just say that.
Anyway, unnecessary cast removed:
commit 4f017daab51f4281e9d859b2491efe83fd199692 Author: Mark Wielaard mjw@redhat.com Date: Tue Aug 27 11:39:52 2013 +0200
gelf_getauxv: Remove unnecessary casts to char *.
Signed-off-by: Mark Wielaard mjw@redhat.com
diff --git a/libelf/ChangeLog b/libelf/ChangeLog index 46d4731..c867010 100644 --- a/libelf/ChangeLog +++ b/libelf/ChangeLog @@ -1,3 +1,7 @@ +2013-08-27 Mark Wielaard mjw@redhat.com + + * gelf_getauxv.c (gelf_getauxv): Remove unnecessary casts to char *. + 2013-08-25 Kurt Roeckx kurt@roeckx.be
* gelf_getauxv.c (gelf_getauxv): Use memcpy instead of pointer diff --git a/libelf/gelf_getauxv.c b/libelf/gelf_getauxv.c index 2a5b6f0..6b91030 100644 --- a/libelf/gelf_getauxv.c +++ b/libelf/gelf_getauxv.c @@ -97,7 +97,7 @@ gelf_getauxv (data, ndx, dst) goto out; }
- memcpy(dst, (char *) data_scn->d.d_buf + ndx * sizeof (GElf_auxv_t), + memcpy(dst, data_scn->d.d_buf + ndx * sizeof (GElf_auxv_t), sizeof (GElf_auxv_t)); }
On Tue, 2013-08-27 at 09:47 -0700, Roland McGrath wrote:
memcpy(dst, (char *) data_scn->d.d_buf + ndx * sizeof (GElf_auxv_t),
memcpy(dst, data_scn->d.d_buf + ndx * sizeof (GElf_auxv_t), sizeof (GElf_auxv_t));This still has a missing space and the indentation is off.
Please next time do point out exactly where (or provide a patch). Missing or extra whitespace is always hard to see. I really didn't spot it. But Josh helped me see the light. I am assuming you mean:
diff --git a/libelf/gelf_getauxv.c b/libelf/gelf_getauxv.c index 6b91030..a2f04e7 100644 --- a/libelf/gelf_getauxv.c +++ b/libelf/gelf_getauxv.c @@ -97,8 +97,8 @@ gelf_getauxv (data, ndx, dst) goto out; }
- memcpy(dst, data_scn->d.d_buf + ndx * sizeof (GElf_auxv_t), - sizeof (GElf_auxv_t)); + memcpy (dst, data_scn->d.d_buf + ndx * sizeof (GElf_auxv_t), + sizeof (GElf_auxv_t)); }
result = dst;
I'll check that in,
Mark
elfutils-devel@lists.fedorahosted.org
-
Florian Weimer -
Kurt Roeckx -
Mark Wielaard -
Mike Frysinger -
Roland McGrath