diff mbox series

dwarf-reader: Fix bloom filter access in GNU_HASH section

Message ID 86d096sm90.fsf@seketeli.org
State Committed
Headers
Series dwarf-reader: Fix bloom filter access in GNU_HASH section |

Commit Message

Dodji Seketeli March 20, 2020, 10:19 p.m. UTC 
  Hello,

The bloom filter of the GNU_HASH section of binaries is made of words
(bitmasks) that are either 32-bits for ELFCLASS32 binaries or 64-bits
for ELFCLASS64 binaries.

By using the GElf_Word type to hold the content of each bitmask we
assume the bitmask to be of a size of at most 'sizeof(Elf64_Word)'.
Unfortunately, the name Elf64_Word is a bit misleading because it's a
32-bits wide type (uint32_t).  That type is thus too short to hold an
entire bitmask for 64-bits binaries.

I won't give too many details here about how the bloom filter works as
it's described in details in the documentation for the GNU_HASH
section at http://www.linker-aliens.org/blogs/ali/entry/gnu_hash_elf_sections/.

Suffice it to say that in practise, we were comparing the least
significant bytes of a 64-bits bloom bitmask to a 32-bits value.
Depending on if we read those least significant bytes on a big or
little endian, we obviously don't get the same result.  Hence the
recent buid breakage at
https://builder.wildebeest.org/buildbot/#builders/14/builds/352 where
the runtestlookupsyms test fails.

This patch thus changes the code of lookup_symbol_from_gnu_hash_tab to
use a 64-bits type to hold the value of the bloom bitmask.  That way,
we never have any information loss.  The function bloom_word_at is
also changed to read the bloom bitmask as a 64-bits value when looking
at an ELFCLASS64 binary and to always return a 64-bits value.

It also adds a test to access the bloom filter of an ELFCLASS32
binary.

	* src/abg-dwarf-reader.cc (bloom_word_at): Properly read an
	element from the bloom bitmasks array of 32-bits values as a
	64-bits value in a portable way.  Always return a 64 bits value.
	(lookup_symbol_from_gnu_hash_tab): Use a 64-bits value to store
	the bloom bitmask.
	* tests/data/test-lookup-syms/test1-32bits.so: New test input,
	compiled as a 32bits binary to test for ELFCLASS32 binaries.
	* tests/data/test-lookup-syms/test1.c.compiling.txt: Documentation
	about how to compile the test1[-21bits].so files.
	* tests/data/Makefile.am: Add the new test material above to
	source distribution.
        * tests/test-lookup-syms.cc (in_out_specs): Add the
	test1-32bits.so test case to this test harness.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-dwarf-reader.cc                           |  24 ++++++++++++++++------
 tests/data/Makefile.am                            |   2 ++
 tests/data/test-lookup-syms/test1-32bits.so       | Bin 0 -> 7800 bytes
 tests/data/test-lookup-syms/test1.c.compiling.txt |   9 ++++++++
 tests/test-lookup-syms.cc                         |   7 +++++++
 5 files changed, 36 insertions(+), 6 deletions(-)
 create mode 100755 tests/data/test-lookup-syms/test1-32bits.so
 create mode 100644 tests/data/test-lookup-syms/test1.c.compiling.txt
diff mbox series

Patch

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index 5bc8a15..10ca123 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -1850,20 +1850,27 @@  get_elf_class_size_in_bytes(Elf* elf_handle)
 }
 
 /// Get a given word of a bloom filter, referred to by the index of
-/// the word.  The word size depends on the current elf class and this
-/// function abstracts that nicely.
+/// the word.
+///
+/// The bloom word size depends on the current elf class (32 bits for
+/// an ELFCLASS32 or 64 bits for an ELFCLASS64 one) and this function
+/// abstracts that nicely.
 ///
 /// @param elf_handle the elf handle to use.
 ///
 /// @param bloom_filter the bloom filter to consider.
 ///
 /// @param index the index of the bloom filter to return.
-static GElf_Word
+///
+/// @return a 64 bits work containing the bloom word found at index @p
+/// index.  Note that if we are looking at an ELFCLASS32 binary, the 4
+/// most significant bytes of the result are going to be zero.
+static Elf64_Xword
 bloom_word_at(Elf*		elf_handle,
 	      Elf32_Word*	bloom_filter,
 	      size_t		index)
 {
-  GElf_Word result = 0;
+  Elf64_Xword result = 0;
   GElf_Ehdr h;
   ABG_ASSERT(gelf_getehdr(elf_handle, &h));
   int c;
@@ -1876,7 +1883,7 @@  bloom_word_at(Elf*		elf_handle,
       break ;
     case ELFCLASS64:
       {
-	GElf_Word* f= reinterpret_cast<GElf_Word*>(bloom_filter);
+	Elf64_Xword* f= reinterpret_cast<Elf64_Xword*>(bloom_filter);
 	result = f[index];
       }
       break;
@@ -2025,7 +2032,12 @@  lookup_symbol_from_gnu_hash_tab(const environment*		env,
   // filter, in bits.
   int c = get_elf_class_size_in_bytes(elf_handle) * 8;
   int n =  (h1 / c) % ht.bf_nwords;
-  GElf_Word bitmask = (1ul << (h1 % c)) | (1ul << (h2 % c));
+  // The bitmask of the bloom filter has a size of either 32-bits on
+  // ELFCLASS32 binaries or 64-bits on ELFCLASS64 binaries.  So we
+  // need a 64-bits type to hold the bitmap, hence the Elf64_Xword
+  // type used here.  When dealing with 32bits binaries, the upper
+  // bits of the bitmask will be zero anyway.
+  Elf64_Xword bitmask = (1ul << (h1 % c)) | (1ul << (h2 % c));
 
   // Test if the symbol is *NOT* present in this ELF file.
   if ((bloom_word_at(elf_handle, ht.bloom_filter, n) & bitmask) != bitmask)
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index e9fdad1..cf7792f 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -1324,6 +1324,8 @@  test-lookup-syms/test02-report.txt	\
 test-lookup-syms/test1.c		\
 test-lookup-syms/test1.version-script	\
 test-lookup-syms/test1.so		\
+test-lookup-syms/test1-32bits.so	\
+test-lookup-syms/test1.c.compiling.txt  \
 test-lookup-syms/test1-1-report.txt	\
 test-lookup-syms/test1-2-report.txt	\
 test-lookup-syms/test1-3-report.txt	\
diff --git a/tests/data/test-lookup-syms/test1-32bits.so b/tests/data/test-lookup-syms/test1-32bits.so
new file mode 100755
index 0000000000000000000000000000000000000000..119cee41f49cd5344fb71b4e0b8d9c95b3afc43f
GIT binary patch
literal 7800
zcmeHMYitzP6~41$!zMNx15OAblqq!*sOs?phhiX!jbE6WM{ScfO3|?E9j^y=cj(S6
zF(^`?3jxb=OBGeA)Tj@ks6V2rk;0!g{bA%ZrBYOZTB+%eKGcX73aVTwZD<2=zwh3;
z_ToYt^|z0G=G=47cVB1jy>sr}7rOeo4Z{$cOkoOY7fm5rAor|PmKI@&#bTZKB;*`g
z+1{xXf{cG5YJ{{`+$TiE8pUxGs0J^J>@mn%*lS@YzYCpAdqu>xz-z8U9$DM=Tn^Lg
z(3s}=_Rqr(_<s=VNB2ZO?B)Hti99mv=h!G~!Ek}-*|k?}?mygjtKnO1!KK{2UmN|=
z+bvu7e3drVN4y+&Ogt&a2c2{}I9)Z`@qzc_o)5e+q*d<Rll{<SA<?ZB$hBbBtI0xQ
zuT~(be`cQUL0^Q9u%Tt5TAV7gFBN~Ad1;<b{D&B*#tX;E<ovu7gvmnaIKt^2-07s<
zf}8Pz&@Bw^?8y3gcQBdCy2^HEi!(HubcVfrGV2`$aK~Y%KdPZ4n+yUs5W~JNvR-P)
z4t#r~2;CrTs^8F@^1=W);_0sbf!^J_oF=<*Hr<SLPhao04yV~}vNx!K@Ews%5hEWn
z;4%LFzV!u@@9tsna|D3s%NU;fPaaKZoAwWFtYiNPWh0)xii77chR8wJ?<`p3-NRY(
z9psTX&xwymvL=$(As3;6X-Z7`_bD-PZct(XH!1PX21?iCS2|F^@u>j)9xuH=Gc$8?
zGOWDvC^AcL#h;ymR5bGjM?Q0w?U82AP6IrAX67sl4WAi*xNd6r%-rAJvC9Kj{tVOc
zsTHWZbiH<>grz4YUaH1({l(IaMb9lNy;b|{zNzujb=B_lSZ&?R#cNk5!bgQH|9x5h
z*Q=uPEUC+%zY!ZdfBxzk<HDsIMP1SOO;+^taC)q>u7Z_ax^{Wu90#N7`+}_Rl&bI8
zo2*Y1>&8nQ3dWt7DAgb}R#A6lHRP2|c)VRA%1}LJ$0mOA7>mjNEe7ED)Mr(F<E3h-
zsOJ9h(hZfWLT{0h!;&>zeFQ3sj(5dNlg7R&os8|Hr{(*2<0IDj-b2v~FjkpcEZFe+
z#A%qeLjDq!?1%gv<S|IjEs}AxUl`BFjaAhZ6{qq3E#$%Xq7&uh97@Ey6V*HJs|qVd
z#ZwP$`E+yLTH4#d2T_;YEZP(Cld)|}Dtms4T2%S{us?klJ4_3-;DH4XEO=nS0}CEl
z@W6rx7Ci7VJur#*>JY<f5My_q2^jA);yi(XZwX{A;&dK`8S5j?lxIZ5hBD@7?8f4M
z|7gbFgu*-z#u4xHJU9aVUdY$LjO!TxF@|JNxD=uj$}-3{@NzKYdPL9u1(d%9Ni4!f
zzYYn6em?t#Fxfu``@bSPvi@)4`CC&@N5^Js9ZtOKtrmNe{kYZK*z|bg#^w#yx_&op
zZBK?OzkWmOdKu874<O|^9wr~qH9>xP)>6Sf3*!tP1K@IC?3=X2J{M)vLB=c8%)?Q3
z?CWTtqSDKUqH-tAi)q#;UE>u3Wfp<LSZX|ySYBl&Rwb4i%NEtFz`@Tpo?2-pHdk$_
zdTeFya%Q(Z`0}=u2E1)n3Nubi@ABA#Xs1q8RXtf{;vH1pr4-s|0msBA*qZTSVhNt|
z`UHbC)N$4xf(=v|<4|k2p4u9V0|mcY9sk?0R()<mGag|v?>>=n^KQW#f|@K0jXW{B
zX=DAy7OOrZ92}#YMZ<`na~slr`heGv@xlh2tCJ}&lk~C;!``SHH1K#GG^CSZQYP!O
zzJIWIs6IH73ugPus+v@f=RJA<LQ*$ELU3*?UA=hDZKZRTsyVN8H6=JlmCh9*1m`r#
zlq&;wI;t4-#WJ63fn>^6pw^z!aoh&IK5OTS5@K>z=fqKWR+Oe(xoXWSohw+~7b!g<
z^*+@t;CUj(`;XRw8J+Zbo+p`drD8vr)wyETUO{Qfm5cUf?dl}R^S`p+uM(CLX0%YM
zXLZceGBO1wV;Pxp=b6he1$Uk~GUbj`u5(A4BU7xFdz#Lvz<m(+Ha#8#I}rD_KSchF
z!GGN2EcF^O+pC2QUGI_9J6Yc1oF}i4D_i@^fWM9+f3mFni_o=y4fyv}=$hD{Xe!Ap
z>OVm0LiBzNyq`Crm-knfr6m4mki4^GlkDGbA@wdn{XOW~-`uGF0-bw0DTW<>8j^cI
z$-w)(1zBTJBdriIbWq=yu|9dHX-T8B%e%_i^0i_$@VuW?ln{;3b^8X|-^u}7%1XT-
z`iWWp-y^<AdqfswSf;Oqeg?W8AMg9lmDzdU_q{Tm<-bv;)BZYiJsy^yEVC2;t9iN@
zDuh8;93IB*<~Tb#`klVsfk9jp&fO6@Va^%iEnxse+IKQpKb6cn>Ci6(PO>;EhWy;2
ztQ)#%dt-BJ%bh{IJoKDop^!Y{xcRVfL<|>_IoC-SbGaiZQl>dThPOfGZKCX8cYpiN
zE~jf(C$BPPcXb+jsGV^`=g^Q7jui6;?bK+rn>sExN+_t8oeO+tB$-e1!qd&CC8!Dc
zsUT3dq{8WZW>@>p-VS79b9cJ7JF403o&Ctdm8tGPPv7os?S0Pf?(TuEL1(aiTVEHt
z#qR(2d)C`-Q`_3gFF~<sx3%fpQr?ixyT7Gfn|Yafr>j_b$9u<nTw&uA5hhcRVL{0e
zoyz;6YiIIBJ5}_u>3T0Mq&AWaMueR{l1E7;!-BFLb_;>$=WkP-s8nQg04=y#78kbk
zi+v~?+8I9-cIb}cDPM~1jzkk+B#jbUQ$^HFP)MCVFa#Lis;s@>%Z?>;-Vi$DhipUG
zc%Q<?)XZVJ!S1Gt85B?EGnmJb>g9)7RGCt#f_qqNSugMEG#rG5^D&A0E5CaDB+9)5
z>#eS=Y_xUCefJ-T^N8d3m)wYSv{lIcj@V6Leg%o+cbWWMq-jG^O2(dTV3sA0F)H~r
zq%{#s+74#CNgTi9WPa6jyUZu^cL~O+O!NCr{u0t`msmZHG}4S?iQ{*lY(;k6Kd2%A
z^J`BW;{kFT(h!nZA7kX_!7RrZ`vi>SPa&;IxBDk#XdL4QGX0>5X=1+u*0@Pv$mKY;
zNg4-h9KSc@c4kDR%gAUHdF&s*Gm<q3Qups=q*<KfE&n|Wsw9nj6`IB|ZXq92klS!S
zgvRIw-ICv1`8$_Hs0ZsK^LxsuhdBAW1r*&DvSgY%zpHGQIDUVt?|_3zyaU!aen)HW
zg8L0{^a<-@yu{xD-$h!tO-WzB2-fv6j=Bz<zF&uFwts&t!wCxtUI1eoO{|Z64a|Dw
zI4od*(`_PKwnaG|r5mY$h^tpg`DCBS|BR%}#G@5BODmAXkQgWEH1%hID>4z0ws8;8
zX_|fkTv~%5wQcP@T=P=3_BB-F><HHw84dvVZ4HX*BbLm$rzwum)`DZ;a?uK=kI)sl
fCh0p(>u}clt8NULI*Deak2UTAgtD#LScdyIPhVNs

literal 0
HcmV?d00001

diff --git a/tests/data/test-lookup-syms/test1.c.compiling.txt b/tests/data/test-lookup-syms/test1.c.compiling.txt
new file mode 100644
index 0000000..6753d63
--- /dev/null
+++ b/tests/data/test-lookup-syms/test1.c.compiling.txt
@@ -0,0 +1,9 @@ 
+To re-compile test1.so (on a 64-bits machine), please do:
+
+gcc -g -shared -Wl,--version-script=test1.version-script -o test1.so test1.c
+
+
+To re-compile test1-32.so, please do:
+
+gcc -g -m32 -shared -Wl,--version-script=test1.version-script -o test1.so test1.c
+
diff --git a/tests/test-lookup-syms.cc b/tests/test-lookup-syms.cc
index 2f4d8f7..077f33b 100644
--- a/tests/test-lookup-syms.cc
+++ b/tests/test-lookup-syms.cc
@@ -74,6 +74,13 @@  InOutSpec in_out_specs[] =
     "output/test-lookup-syms/test1-1-report.txt"
   },
   {
+    "data/test-lookup-syms/test1-32bits.so",
+    "foo",
+    "",
+    "data/test-lookup-syms/test1-1-report.txt",
+    "output/test-lookup-syms/test1-1-report.txt"
+  },
+  {
     "data/test-lookup-syms/test1.so",
     "_foo1",
     "--no-absolute-path",