[PATCH 1/2] libdwfl: handle duplicate ELFs when reporting archives

[PATCH 1/2] libdwfl: handle duplicate ELFs when reporting archives

[vvvvvv]

From: Aleksei Vetrov <vvvvvv@google.com>

When archive is processed in process_archive (libdwfl/offline.c), it
creates an Elf object for each archive member. Then in
process_archive_member it calls process_file to create a Dwfl_Module
through __libdwfl_report_elf.

The ownership of the Elf object is expected to be:

* either transfered to the Dwfl_Module, if __libdwfl_report_elf returns
  not NULL;

* or handled at the end of process_archive_member by calling elf_end.

Moreover, Elf object is expected to be alive, if __libdwfl_report_elf
returns not NULL, because at the end of process_archive_member it
advances to the next member through the elf_next call.

The problem happens when __libdwfl_report_elf encounters Elf with the
same name and content as it seen before. In that case dwfl_report_module
will reuse existing Dwfl_Module object. This leads to a codepath that
calls elf_end on the Elf object, while returning not NULL, breaking the
elf_next call to the next member.

The fix is to destroy m->main.elf instead and put the new Elf object in
the already existing Dwfl_Module.

    * libdwfl/dwfl_report_elf.c (__libdwfl_report_elf): Replace Elf in
      the Dwfl_Module in case of overlapping or duplicate modules to
      prolong its lifetime for subsequent processing.

Signed-off-by: Aleksei Vetrov <vvvvvv@google.com>
---
 libdwfl/dwfl_report_elf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libdwfl/dwfl_report_elf.c b/libdwfl/dwfl_report_elf.c
index 581f4079..58b06aea 100644
--- a/libdwfl/dwfl_report_elf.c
+++ b/libdwfl/dwfl_report_elf.c
@@ -276,7 +276,8 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name,
 	}
       else
 	{
-	  elf_end (elf);
+	  elf_end (m->main.elf);
+	  m->main.elf = elf;
 	  if (m->main_bias != bias
 	      || m->main.vaddr != vaddr || m->main.address_sync != address_sync)
 	    goto overlap;
-- 
2.43.0.rc1.413.gea7ed67945-goog

[PATCH 2/2] tests: Add test for duplicate entries in archive

[vvvvvv]

From: Aleksei Vetrov <vvvvvv@google.com>

Test dwfl-report-offline-memory against an archive that contains
non-relocatable ELFs with the same name and contents.

Signed-off-by: Aleksei Vetrov <vvvvvv@google.com>
---
 tests/run-dwfl-report-offline-memory.sh |   7 +++++++
 tests/test-ar-duplicates.a.bz2          | Bin 0 -> 783 bytes
 2 files changed, 7 insertions(+)
 create mode 100644 tests/test-ar-duplicates.a.bz2

diff --git a/tests/run-dwfl-report-offline-memory.sh b/tests/run-dwfl-report-offline-memory.sh
index 644a45dc..85f43f53 100755
--- a/tests/run-dwfl-report-offline-memory.sh
+++ b/tests/run-dwfl-report-offline-memory.sh
@@ -20,7 +20,14 @@
 testfiles testfile-dwfl-report-elf-align-shlib.so
 testfiles testarchive64.a
 
+# echo "int _start(void) { return 0; }" > test.c
+# gcc test.c -nostdlib -static -o test.o
+# ar -r test-ar-duplicates.a test.o test.o test.o
+# bzip2 -zf test-ar-duplicates.a
+testfiles test-ar-duplicates.a
+
 testrun ${abs_builddir}/dwfl-report-offline-memory ./testfile-dwfl-report-elf-align-shlib.so 1
 testrun ${abs_builddir}/dwfl-report-offline-memory ./testarchive64.a 3
+testrun ${abs_builddir}/dwfl-report-offline-memory ./test-ar-duplicates.a 1
 
 exit 0
diff --git a/tests/test-ar-duplicates.a.bz2 b/tests/test-ar-duplicates.a.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..e8168c6bb813bb05292372d6b856e2ca1163a835
GIT binary patch
literal 783
zcmV+q1MvJpT4*^jL0KkKS*O-t`~U>U|NsC0^v!1T{dI5Uazg*-o^**RKmrI*1V9J?
z0RUpbpefJ;J^%m!H~;_|001&HG5`Po00EJZ00u$8007Vc0g<7Q0000042*yPG7bO$
z27mwzjSPSQ0001FWB>t>Z~y=_003lYWB>pF00Sc+01Sb>Qb4DoG|(YN(+YV{O*J>F
zZ891m=`tHrY3VgR0F2UlO|*o0s(PNL3ovPT-FWk=B;Jux%~F)fXQs4SQOt9gyiE>j
zn6Ga(89!@GYHl-L-dxbs$}&@+(DyVdX=!IB!kuQ))6nI$Da(vh*7{bCREV6#KNqm!
zdu&Mb+M`10By`;OIdVe?-cr&jN>kKstt5_(4hBpYI!%S0RYq!zoTdw-r};8m5SuEI
zu&BS|S>97Ll_Ks%qWX~|eNFX2+)(wDko6K&h@AyU+*7EjB0KJbX(i2-NWr+XprTIu
zB%S!mUnj<+n4}YmKId7>S^5_w!Fy?EWo4tTvdK6U;gV9C5?V$|i8mD;#X<L~F;qz<
z>sVDVQ%g0=kvP>JW-B*XEa@sbQG}Le!OvXnKQ+tOr1|<*S=e&p2DS+rYDV*_vs1e6
zu1U>irf%B#9wjbP_72;9ds3ZsFn8O_*f5OUXClse^_%iUp6rvfsXM*Jq^>8z@U|Q6
z*J1OxE^e`H$%>n8wk=MZHxzfH?{*p^+fvGr{$BN;1J#puO|@+L>>t^ZP1xPOJkJDd
zG&~LmIfeUCo0!>Vz3RBGUhvbk7`3dlX0%-$MeRv3e^oYHE+>J;`q?pM6W?@SQ#QEc
ze~0GQ%}g2{wdp?SsT`$F_Z(%Ds5sw)!*Wg-`sD8<;cAYoXHLwT6*G9*M;hmeoNOvD
zUXsh63|o(x!uDHDwwJQ<E@v84@+{eIJd9#%jkv|P8OF4plK9Q=xDsqeh_q{5laaXh
zUc~3=V;Wm-i(2)goXz8Q9B(USi{5!(`RI9Ax87BbN14d~uU*-b&dJEAg!8=*3()`K
N?ntK!5)}H&|9~F>bnE~C

literal 0
HcmV?d00001

-- 
2.43.0.rc1.413.gea7ed67945-goog

Re: [PATCH 1/2] libdwfl: handle duplicate ELFs when reporting archives

[Mark Wielaard]

Hi Aleksei,

On Fri, Nov 17, 2023 at 10:35:40PM +0000, vvvvvv@google.com wrote:
> When archive is processed in process_archive (libdwfl/offline.c), it
> creates an Elf object for each archive member. Then in
> process_archive_member it calls process_file to create a Dwfl_Module
> through __libdwfl_report_elf.
> 
> The ownership of the Elf object is expected to be:
> 
> * either transfered to the Dwfl_Module, if __libdwfl_report_elf returns
>   not NULL;
> 
> * or handled at the end of process_archive_member by calling elf_end.
> 
> Moreover, Elf object is expected to be alive, if __libdwfl_report_elf
> returns not NULL, because at the end of process_archive_member it
> advances to the next member through the elf_next call.
> 
> The problem happens when __libdwfl_report_elf encounters Elf with the
> same name and content as it seen before. In that case dwfl_report_module
> will reuse existing Dwfl_Module object. This leads to a codepath that
> calls elf_end on the Elf object, while returning not NULL, breaking the
> elf_next call to the next member.
> 
> The fix is to destroy m->main.elf instead and put the new Elf object in
> the already existing Dwfl_Module.
> 
>     * libdwfl/dwfl_report_elf.c (__libdwfl_report_elf): Replace Elf in
>       the Dwfl_Module in case of overlapping or duplicate modules to
>       prolong its lifetime for subsequent processing.

Thanks for that analysis and proposed solution. The ownership
reasoning makes sense. But I do have one question.

> diff --git a/libdwfl/dwfl_report_elf.c b/libdwfl/dwfl_report_elf.c
> index 581f4079..58b06aea 100644
> --- a/libdwfl/dwfl_report_elf.c
> +++ b/libdwfl/dwfl_report_elf.c
> @@ -276,7 +276,8 @@ __libdwfl_report_elf (Dwfl *dwfl, const char *name, const char *file_name,
>  	}
>        else
>  	{
> -	  elf_end (elf);
> +	  elf_end (m->main.elf);
> +	  m->main.elf = elf;
>  	  if (m->main_bias != bias
>  	      || m->main.vaddr != vaddr || m->main.address_sync != address_sync)
>  	    goto overlap;

If we goto overlap here don't we still have a problem? overlap will
set m->gc = true; and return NULL. So the caller will think they
still owns the elf handle and will probably close it. But then when
the module is GCed in dwfl_report_end it will close the elf handle
again.

Should we instead move the elf_end and reassignment of main.elf to
after this if statement?

Thanks,

Mark

Re: [PATCH 2/2] tests: Add test for duplicate entries in archive

[Mark Wielaard]

Hi Aleksei,

On Fri, Nov 17, 2023 at 10:35:41PM +0000, vvvvvv@google.com wrote:
> Test dwfl-report-offline-memory against an archive that contains
> non-relocatable ELFs with the same name and contents.

Nice testcase. Do note that you also have to add the new test file to
EXTRA_DIST so it actually gets into the dist.

            * tests/test-ar-duplicates.a.bz2: New test file.
            * tests/run-dwfl-report-offline-memory.sh: Test new
            test-ar-duplicates.a.bz2.
            * tests/Makefile.am (EXTRA_DIST): Add test-ar-duplicates.a.bz2.

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 7fb8efb1..80f6cb59 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -632,7 +632,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
             run-nvidia-extended-linemap-libdw.sh run-nvidia-extended-linemap-readelf.sh \
             testfile_nvidia_linemap.bz2 \
             testfile-largealign.o.bz2 run-strip-largealign.sh \
-            run-funcretval++11.sh
+            run-funcretval++11.sh \
+            test-ar-duplicates.a.bz2

make distcheck catches these things by creating a dist and doing a
build and run various tests and make check.

Cheers,

Mark

Re: [PATCH 2/2] tests: Add test for duplicate entries in archive

[Aleksei Vetrov]

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

Hello Mark,

On Sat, Nov 18, 2023 at 10:50 PM Mark Wielaard <mark@klomp.org> wrote:
> Do note that you also have to add the new test file to
> EXTRA_DIST so it actually gets into the dist.

Thanks, will do in [PATCH v2].

Re: [PATCH 1/2] libdwfl: handle duplicate ELFs when reporting archives

[Aleksei Vetrov]

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

Hello Mark,

On Sat, Nov 18, 2023 at 10:47 PM Mark Wielaard <mark@klomp.org> wrote:
> If we goto overlap here don't we still have a problem? overlap will
> set m->gc = true; and return NULL. So the caller will think they
> still owns the elf handle and will probably close it. But then when
> the module is GCed in dwfl_report_end it will close the elf handle
> again.

Thank you for noticing! Yes, this is oversight from my side.

> Should we instead move the elf_end and reassignment of main.elf to
> after this if statement?

Will fix exactly like this in [PATCH v2].