AR archive iteration broken?

[Richard Guenther]

When iterating over AR archive members like

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <libelf.h>

int main(int argc, char **argv)
{
  int fd;
  Elf *e, *parent;
  elf_version (EV_CURRENT);
  fd = open (argv[1], O_RDONLY);
  parent = elf_begin (fd, ELF_C_READ, NULL);
  while ((e = elf_begin (fd, ELF_C_READ, parent)) != NULL)
    {
      Elf_Arhdr *h = elf_getarhdr (e);
      fprintf (stderr, "%llu: %s\n", elf_getaroff (e), h->ar_name);
      elf_next (e);
      elf_end (e);
    }

  return 0;
}

the iteration does not stop but instead the last archive member
keeps being opened again and again and elf_getaroff keeps
increasing.

Take

/tmp$ ar t t.a
t2.o
t1.o
/tmp$ ./t t.a 2>&1 | head
8: /
120: t2.o
1658: t1.o
3494: t1.o
5330: t1.o
7166: t1.o
9002: t1.o
10838: t1.o
12674: t1.o
14510: t1.o

Observed with elfutils 0.143.

Richard.

-- 
Richard Guenther <rguenther(a)suse.de&gt;
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex

[Mark Wielaard]

On Sun, 2009-11-01 at 12:29 +0100, Richard Guenther wrote:
>  When iterating over AR archive members like
>  
>    while ((e = elf_begin (fd, ELF_C_READ, parent)) != NULL)
>      {
>        Elf_Arhdr *h = elf_getarhdr (e);
>        fprintf (stderr, "%llu: %s\n", elf_getaroff (e), h->ar_name);
>        elf_next (e);
>        elf_end (e);
>      }
>
>  the iteration does not stop but instead the last archive member
>  keeps being opened again and again and elf_getaroff keeps
>  increasing. 
This seems to be caused by elf_next() not marking the archive header as
unusable (like elf_rand() does) when there is no next element. The
following fixes it for me:

diff --git a/libelf/elf_next.c b/libelf/elf_next.c
index fbfb272..d73fac6 100644
--- a/libelf/elf_next.c
+++ b/libelf/elf_next.c
@@ -83,6 +83,11 @@ elf_next (elf)
 
   /* Get the next archive header.  */
   ret = __libelf_next_arhdr_wrlock (parent) != 0 ? ELF_C_NULL : elf->cmd;
+  if (ret == ELF_C_NULL)
+    {
+      /* Mark the archive header as unusable.  */
+      parent->state.ar.elf_ar_hdr.ar_name = NULL;
+    }
 
   rwlock_unlock (parent->lock);
 
With that in place elf_begin() will see ar_name == NULL and that
__libelf_next_arhdr_wrlock() fails.

Cheers,

Mark

[Roland McGrath]

I tend to think the right fix is instead to have elf_next leave
ar.offset where it was in the failure case, so it changes nothing
at all.  That won't make Richard's program work like he expected,
but it will make the end-of-archive case behave less confusingly.
I can't see any reason to think the ever-more-bogus offset behavior
is not a bug.  But we need to hear Ulrich's opinion on the fix.


Thanks,
Roland

[Mark Wielaard]

On Sun, 2009-11-01 at 19:36 -0800, Roland McGrath wrote:
>  I tend to think the right fix is instead to have elf_next leave
>  ar.offset where it was in the failure case, so it changes nothing
>  at all. 
That actually is already the case, for the public ar offset the user
gets from get_arroff(), even when elf_next() failed. Only the internal
state.ar.offset is changed, which isn't used till the user calls
elf_begin/elf_getarhdr(). So what my patch does is make sure it isn't
used at all when elf_next() failed and elf_begin() or elf_getarhdr() is
called subsequently (and it has the nice property of doing what Richard
expected).

I admit I also didn't know you were supposed to feed to result of
elf_next() into elf_begin(). That does make sense though. But it would
be nice if elf_begin() indicated end of archive even when given
ELF_C_READ, especially if there are other implementations doing that.

Cheers,

Mark

[Roland McGrath]

>  I admit I also didn't know you were supposed to feed to result
> of
>  elf_next() into elf_begin(). That does make sense though. But it would
>  be nice if elf_begin() indicated end of archive even when given
>  ELF_C_READ, especially if there are other implementations doing that. 
Yes, I agree that seems reasonable.  
Do you have a patch/branch against current master that does that?


Thanks,
Roland

[Mark Wielaard]

On Thu, 2009-11-05 at 12:17 -0800, Roland McGrath wrote:
>  > I admit I also didn't know you were supposed to feed to
> result of
>  > elf_next() into elf_begin(). That does make sense though. But it would
>  > be nice if elf_begin() indicated end of archive even when given
>  > ELF_C_READ, especially if there are other implementations doing that.
>  
>  Yes, I agree that seems reasonable.  
>  Do you have a patch/branch against current master that does that? 
I was under the impression that is what my patch does. It marks the
archive header as unusable (ar_name = NULL) in elf_next (), so
elf_begin() will see that ar_name == NULL and that
__libelf_next_arhdr_wrlock() fails, so it will return NULL, from
dup_elf(), to indicate end of archive when given C_READ.

Cheers,

Mark

[Roland McGrath]

>  I was under the impression that is what my patch does. It marks the
>  archive header as unusable (ar_name = NULL) in elf_next (), so
>  elf_begin() will see that ar_name == NULL and that
>  __libelf_next_arhdr_wrlock() fails, so it will return NULL, from
>  dup_elf(), to indicate end of archive when given C_READ. 
Ok, that sounds fine.  Go ahead and push it, without gratuitous braces,
and with ChangeLog entry.

Thanks,
Roland

[Roland McGrath]

Are you aware of a different libelf implementation (e.g. Solaris) where
your test program works as you expect?

The normal idiom for archive iteration is to use the return value from
elf_next, rather than ignoring it.  It returns ELF_C_NULL when there is no
later element.  You don't need to test that return value directly unless
you want to.  The normal idiom is to pass that return value in to
elf_begin, and ELF_C_NULL to elf_begin just returns NULL.  e.g.:

  Elf_Cmd cmd = ELF_C_READ;
  while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
    {
      Elf_Arhdr *arhdr = elf_getarhdr (subelf);
      ... work on subelf ...
      /* Get next archive element.  */
      cmd = elf_next (subelf);
      if (elf_end (subelf) != 0)
	... diagnose error ...;
    }


Thanks,
Roland

[Richard Guenther]

On Sun, 1 Nov 2009, Roland McGrath wrote:

>  Are you aware of a different libelf implementation (e.g. Solaris)
> where
>  your test program works as you expect? 
It works with libelf from Michael Riepe and I got the idiom from
some Solaris libelf tutorial (can't find it right now).

>  The normal idiom for archive iteration is to use the return value
> from
>  elf_next, rather than ignoring it.  It returns ELF_C_NULL when there is no
>  later element.  You don't need to test that return value directly unless
>  you want to.  The normal idiom is to pass that return value in to
>  elf_begin, and ELF_C_NULL to elf_begin just returns NULL.  e.g.:
>  
>    Elf_Cmd cmd = ELF_C_READ;
>    while ((subelf = elf_begin (fd, cmd, elf)) != NULL)
>      {
>        Elf_Arhdr *arhdr = elf_getarhdr (subelf);
>        ... work on subelf ...
>        /* Get next archive element.  */
>        cmd = elf_next (subelf);
>        if (elf_end (subelf) != 0)
>  	... diagnose error ...;
>      } 
Ok, I'll see if that works across the different libelfs I have access to.
Documentation seems to be a scarce resource here...

Thanks,
Richard.