Re: [PATCH v4 4/6] elf: Add _bfd_elf_mmap_section and _bfd_elf_munmap_section_contents

["H.J. Lu" <hjl.tools@gmail.com>]

On Thu, Mar 7, 2024 at 4:41 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, Mar 06, 2024 at 03:23:59PM -0800, H.J. Lu wrote:
> > +void
> > +_bfd_elf_munmap_section_contents (asection *sec ATTRIBUTE_UNUSED,
> > +                               void *contents)
> > +{
> > +  /* NB: Since _bfd_elf_munmap_section_contents is called like free,
> > +     CONTENTS may be NULL.  */
> > +  if (contents == NULL)
> > +    return;
> > +
> > +#ifdef USE_MMAP
> > +  if (sec->mmapped_p)
> > +    {
> > +      /* NB: Don't free CONTENTS if it has been cached.  */
> > +      if (elf_section_data (sec)->this_hdr.contents == contents)
> > +     return;
>
> Why the above test?  This function must have such a test before it is
> called if the contents might be cached.  Hmm, unless you have some
> place where contents are cached only when mmapped?  (I didn't see such
> a case.)

When _bfd_elf_munmap_section_contents is called from
_bfd_elf_parse_eh_frame, the section contents may be cached.
I updated the comments to clarify it.

> > +      /* When bfd_mmap_local returns (void *) -1 on an unknown input,
> > +      CONTENTS is malloced and CONTENTS_ADDR is set to NULL.  */
>
> The above comment adds confusion.  As far as I can see there isn't a
> case where bfd_mmap_local returns -1 causing contents to be malloc'd.
> Delete the comment, or mention _bfd_elf_mmap_section behaviour here.

Changed to

     /* When _bfd_elf_mmap_section_contents returns CONTENTS as
         malloced, CONTENTS_ADDR is set to NULL.  */

> BTW, why does this function have _contents in the name while
> _bfd_elf_mmap_section does not?

Changed to _bfd_elf_mmap_section_contents.

> > +      if (elf_section_data (sec)->contents_addr != NULL)
> > +     {
> > +       /* NB: CONTENTS_ADDR and CONTENTS_SIZE must be valid.  */
> > +       if (munmap (elf_section_data (sec)->contents_addr,
> > +                   elf_section_data (sec)->contents_size) != 0)
> > +         abort ();
> > +       sec->mmapped_p = 0;
> > +       sec->contents = NULL;
> > +       elf_section_data (sec)->contents_addr = NULL;
> > +       elf_section_data (sec)->contents_size = 0;
> > +       return;
> > +     }
> > +    }
> > +#endif
> > +
> > +  free (contents);
> > +}
>
> > diff --git a/bfd/libbfd.c b/bfd/libbfd.c
> > index 237f91e5c97..c847c4f0180 100644
> > --- a/bfd/libbfd.c
> > +++ b/bfd/libbfd.c
> > @@ -21,6 +21,7 @@
> >
> >  #include "sysdep.h"
> >  #include "bfd.h"
> > +#include "elf-bfd.h"
> >  #include "libbfd.h"
> >  #include "objalloc.h"
> >
> > @@ -1202,6 +1203,19 @@ _bfd_generic_get_section_contents (bfd *abfd,
> >        return false;
> >      }
> >
> > +#ifdef USE_MMAP
> > +  if (section->mmapped_p
> > +      && (section->contents != NULL || location != NULL))
> > +    {
> > +      _bfd_error_handler
> > +     /* xgettext:c-format */
> > +     (_("%pB: mapped section %pA has non-NULLL buffer"),
>
> typo

Fixed.

>
> > +      abfd, section);
> > +      bfd_set_error (bfd_error_invalid_operation);
> > +      return false;
> > +    }
> > +#endif
> > +
> >    sz = bfd_get_section_limit_octets (abfd, section);
> >    if (offset + count < count
> >        || offset + count > sz
> > @@ -1214,8 +1228,49 @@ _bfd_generic_get_section_contents (bfd *abfd,
> >        return false;
> >      }
> >
> > -  if (bfd_seek (abfd, section->filepos + offset, SEEK_SET) != 0
> > -      || bfd_read (location, count, abfd) != count)
> > +  if (bfd_seek (abfd, section->filepos + offset, SEEK_SET) != 0)
> > +    return false;
> > +
> > +#ifdef USE_MMAP
> > +  if (section->mmapped_p)
> > +    {
> > +      if (location != 0
> > +       || bfd_get_flavour (abfd) != bfd_target_elf_flavour)
> > +     abort ();
> > +
> > +      int prot = ((section->reloc_count == 0)
> > +               ? PROT_READ : PROT_READ | PROT_WRITE);
> > +
> > +      location = bfd_mmap_local
> > +     (abfd, count, prot, &elf_section_data (section)->contents_addr,
> > +      &elf_section_data (section)->contents_size);
> > +
> > +      if (location == NULL)
> > +     return false;
> > +
> > +      /* Check for the unknown input file.  */
>
> Not "the unknown input file" but rather "iovec not supporting mmap".

Fixed.

> > +      if (location != (void *) -1)
> > +     {
> > +       section->contents = location;
> > +       return true;
> > +     }
> > +
> > +      /* Malloc the buffer and call bfd_read.  */
> > +      location = (bfd_byte *) bfd_malloc (count);
> > +      if (location == NULL)
> > +     {
> > +       if (bfd_get_error () == bfd_error_no_memory)
> > +         _bfd_error_handler
> > +           /* xgettext:c-format */
> > +           (_("error: %pB(%pA) is too large (%#" PRIx64 " bytes)"),
> > +            abfd, section, (uint64_t) count);
> > +       return false;
> > +     }
> > +      section->contents = location;
> > +    }
> > +#endif
> > +
> > +  if (bfd_read (location, count, abfd) != count)
> >      return false;
> >
> >    return true;
>
> OK with those things fixed.
>
> --
> Alan Modra
> Australia Development Lab, IBM

Thanks.

-- 
H.J.