Re: [PATCH 11/24] elf: Add a way to read segment information from an ELF file

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 08/02/2022 21:49, Simon Glass wrote:
> Add a function which reads the segments and the entry address.
> 
> Also fix a comment nit in the tests while we are here.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/binman/elf.py      | 37 +++++++++++++++++++++++++++++++++++++
>  tools/binman/elf_test.py | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/binman/elf.py b/tools/binman/elf.py
> index de2bb4651f..2b83ac1876 100644
> --- a/tools/binman/elf.py
> +++ b/tools/binman/elf.py
> @@ -20,6 +20,7 @@ from patman import tout
>  ELF_TOOLS = True
>  try:
>      from elftools.elf.elffile import ELFFile
> +    from elftools.elf.elffile import ELFError
>      from elftools.elf.sections import SymbolTableSection
>  except:  # pragma: no cover
>      ELF_TOOLS = False
> @@ -369,3 +370,39 @@ def UpdateFile(infile, outfile, start_sym, end_sym, insert):
>      newdata += data[syms[end_sym].offset:]
>      tools.WriteFile(outfile, newdata)
>      tout.Info('Written to offset %#x' % syms[start_sym].offset)
> +
> +def read_segments(data):
> +    """Read segments from an ELF file
> +
> +    Args:
> +        data (bytes): Contents of file
> +
> +    Returns:
> +        tuple:
> +            list of segments, each:
> +                int: Segment number (0 = first)
> +                int: Start address of segment in memory
> +                bytes: Contents of segment
> +            int: entry address for image
> +
> +    Raises:
> +        ValueError: elftools is not available

... or input data is not a correct ELF file?

> +    """
> +    if not ELF_TOOLS:
> +        raise ValueError('Python elftools package is not available')

I see something like ModuleNotFoundError("No module named 'elftools'")
when I try to import an unavailable module, so maybe this could match that.

> +    with io.BytesIO(data) as inf:
> +        try:
> +            elf = ELFFile(inf)
> +        except ELFError as err:
> +            raise ValueError(err)

Could also be: raise ValueError("Not an ELF file") from err

But I guess you want err's message here to match on it in tests. (It's
also possible but slightly inconvenient with __cause__ when using
raise-from)

> +        entry = elf.header['e_entry']
> +        segments = []
> +        for i in range(elf.num_segments()):
> +            segment = elf.get_segment(i)
> +            if segment['p_type'] != 'PT_LOAD' or not segment['p_memsz']:

I can't say I fully understand ELF details, is it obvious in context
that a function named read_segments() would only return these segments,
or should the name be explicit about it e.g. read_loadable_segments()?

> +                skipped = 1  # To make code-coverage see this line
> +                continue
> +            start = segment['p_offset']
> +            rend = start + segment['p_filesz']
> +            segments.append((i, segment['p_paddr'], data[start:rend]))
> +    return segments, entry
> diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
> index f727258487..369260c17a 100644
> --- a/tools/binman/elf_test.py
> +++ b/tools/binman/elf_test.py
> @@ -56,8 +56,8 @@ class FakeSection:
>  def BuildElfTestFiles(target_dir):
>      """Build ELF files used for testing in binman
>  
> -    This compiles and links the test files into the specified directory. It the
> -    Makefile and source files in the binman test/ directory.
> +    This compiles and links the test files into the specified directory. It uses
> +    the Makefile and source files in the binman test/ directory.
>  
>      Args:
>          target_dir: Directory to put the files into
> @@ -258,6 +258,33 @@ class TestElf(unittest.TestCase):
>          offset = elf.GetSymbolFileOffset(fname, ['missing_sym'])
>          self.assertEqual({}, offset)
>  
> +    def test_read_segments(self):
> +        """Test for read_segments()"""
> +        if not elf.ELF_TOOLS:
> +            self.skipTest('Python elftools not available')
> +        fname = self.ElfTestFile('embed_data')
> +        segments, entry = elf.read_segments(tools.ReadFile(fname))
> +
> +    def test_read_segments_fail(self):
> +        """Test for read_segments() without elftools"""
> +        try:
> +            old_val = elf.ELF_TOOLS
> +            elf.ELF_TOOLS = False
> +            fname = self.ElfTestFile('embed_data')
> +            with self.assertRaises(ValueError) as e:
> +                elf.read_segments(tools.ReadFile(fname))
> +            self.assertIn('Python elftools package is not available',
> +                          str(e.exception))
> +        finally:
> +            elf.ELF_TOOLS = old_val
> +
> +    def test_read_segments_bad_data(self):
> +        """Test for read_segments() with an invalid ELF file"""
> +        fname = self.ElfTestFile('embed_data')
> +        with self.assertRaises(ValueError) as e:
> +            elf.read_segments(tools.GetBytes(100, 100))
> +        self.assertIn('Magic number does not match', str(e.exception))
> +
>  
>  if __name__ == '__main__':
>      unittest.main()