[patchv2] PR gdb/17968 - [ppc64] SEGV in ppc64_elf_get_synthetic_symtab reading a separate debug file
Commit Message
Hi Martin,
[ppc64] SEGV in ppc64_elf_get_synthetic_symtab reading a separate debug file
https://sourceware.org/bugzilla/show_bug.cgi?id=17968
Martin Sebor 2015-02-16 00:42:58 CET
# The problem appears to be due to the change to gdb/elfread.c introduced in the commit below:
# # commit 63524580f8372e38a6a62fd875a4252068c31150
# # Author: Jan Kratochvil <jan.kratochvil@redhat.com>
# # Date: Sun Apr 17 18:38:46 2011 +0000
# # gdb/
# # Fix convert_code_addr_to_desc_addr for ppc64 files after eu-strip.
# # * elfread.c (elf_symfile_read): New variable synth_abfd, pass it to
# # bfd_get_synthetic_symtab.
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=63524580f8372e38a6a62fd875a4252068c31150
I find the patch of mine above right even for object files and your patch only
avoided hitting the stale memory reference by disabling proper .opd parsing.
Valgrind on F-21 x86_64 host showed me more clear what is the problem:
Reading symbols from /home/jkratoch/t/cordic.ko...Reading symbols from /home/jkratoch/t/cordic.ko.debug...=================================================================
==22763==ERROR: AddressSanitizer: heap-use-after-free on address 0x6120000461c8 at pc 0x150cdbd bp 0x7fffffffc7e0 sp 0x7fffffffc7d0
READ of size 8 at 0x6120000461c8 thread T0
#0 0x150cdbc in ppc64_elf_get_synthetic_symtab /home/jkratoch/redhat/gdb-test-asan/bfd/elf64-ppc.c:3282
#1 0x8c5274 in elf_read_minimal_symbols /home/jkratoch/redhat/gdb-test-asan/gdb/elfread.c:1205
#2 0x8c55e7 in elf_symfile_read /home/jkratoch/redhat/gdb-test-asan/gdb/elfread.c:1268
[...]
0x6120000461c8 is located 264 bytes inside of 288-byte region [0x6120000460c0,0x6120000461e0)
freed by thread T0 here:
#0 0x7ffff715454f in __interceptor_free (/lib64/libasan.so.1+0x5754f)
#1 0xde9cde in xfree common/common-utils.c:98
#2 0x9a04f7 in do_my_cleanups common/cleanups.c:155
#3 0x9a05d3 in do_cleanups common/cleanups.c:177
#4 0x8c538a in elf_read_minimal_symbols /home/jkratoch/redhat/gdb-test-asan/gdb/elfread.c:1229
#5 0x8c55e7 in elf_symfile_read /home/jkratoch/redhat/gdb-test-asan/gdb/elfread.c:1268
[...]
previously allocated by thread T0 here:
#0 0x7ffff71547c7 in malloc (/lib64/libasan.so.1+0x577c7)
#1 0xde9b95 in xmalloc common/common-utils.c:41
#2 0x8c4da2 in elf_read_minimal_symbols /home/jkratoch/redhat/gdb-test-asan/gdb/elfread.c:1147
#3 0x8c55e7 in elf_symfile_read /home/jkratoch/redhat/gdb-test-asan/gdb/elfread.c:1268
[...]
SUMMARY: AddressSanitizer: heap-use-after-free /home/jkratoch/redhat/gdb-test-asan/bfd/elf64-ppc.c:3282 ppc64_elf_get_synthetic_symtab
[...]
==22763==ABORTING
A similar case a few lines later I have fixed in 2010 by:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=3f1eff0a2c7f0e7078f011f55b8e7f710aae0cc2
Your testcase may be fine although it does not reproduce the crash on x86_64
host. Therefore attaching the binaries themselves to make the crash more
reproducible.
Although... my testcase does not always reproduce it but at least a bit:
* GDB without ppc64 target (even as a secondary one) is reported as "untested"
* ASAN-built GDB with ppc64 target always crashes (and PASSes with this fix)
* unpatched non-ASAN-built GDB with ppc64 target crashes from commandline
* unpatched non-ASAN-built GDB with ppc64 target PASSes from runtest (?)
Jan
On Wed, 18 Feb 2015 00:40:02 +0100, Martin Sebor wrote:
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 0bd0792..288ec7d 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2015-02-16 Martin Sebor <msebor@redhat.com>
> +
> + PR gdb/17968
> + * elread.c (elf_symfile_read): Use synth_abfd only for shared and
elfread
> + executable objects (but not relocatable files).
> +
> 2015-02-13 Doug Evans <dje@google.com>
>
> * cp-namespace.c (cp_basic_lookup_symbol): Rename parameter
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index 65c63f0..b1074e0 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1183,19 +1183,22 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
> elf_rel_plt_read (objfile, dyn_symbol_table);
> }
>
> - /* Contrary to binutils --strip-debug/--only-keep-debug the strip command from
> - elfutils (eu-strip) moves even the .symtab section into the .debug file.
> + /* Both the Binutils strip command and the Elfutils eu-strip command
> + remove the .symtab section from executables or dynamic object files
> + (but not from reloacatable object files like Linux kernel modules)
relocatable
> + and move it into the .debug file.
>
> bfd_get_synthetic_symtab on ppc64 for each function descriptor ELF symbol
> 'name' creates a new BSF_SYNTHETIC ELF symbol '.name' with its code
> - address. But with eu-strip files bfd_get_synthetic_symtab would fail to
> - read the code address from .opd while it reads the .symtab section from
> - a separate debug info file as the .opd section is SHT_NOBITS there.
> + address. But with stripped files without the .symtab section
> + bfd_get_synthetic_symtab would fail to read the code address from .opd
> + while it reads the .symtab section from a separate debug info file as
> + the .opd section is SHT_NOBITS there.
>
> With SYNTH_ABFD the .opd section will be read from the original
> backlinked binary where it is valid. */
>
> - if (objfile->separate_debug_objfile_backlink)
> +if (abfd->flags & (EXEC_P | DYNAMIC) && objfile->separate_debug_objfile_backlink)
missing/removed indentation
> synth_abfd = objfile->separate_debug_objfile_backlink->obfd;
> else
> synth_abfd = abfd;
[...]
> +if { [# Copy debug info from the relocatable file into a separate .dbg file.
> + verify "objcopy --only-keep-debug $binfile.bu $binfile.bu.dbg"] &&
> + [# Strip debug (and other unneeded) sections from the relocatable
> + # file.
> + verify "strip --strip-debug --strip-unneeded $binfile.bu"] &&
> + [# Insert the .gnu_debuglink section into the relocatable file
> + # pointing at the separate .dbg file.
> + verify "objcopy --add-gnu-debuglink $binfile.bu.dbg $binfile.bu"]} {
I haven't tried that but lib/gdb.exp contains gdb_gnu_strip_debug(), would not
it be applicable here?
Jan
gdb/ChangeLog
2015-02-25 Jan Kratochvil <jan.kratochvil@redhat.com>
* elfread.c (elf_read_minimal_symbols): Use bfd_alloc for
bfd_canonicalize_symtab.
gdb/testsuite/ChangeLog
2015-02-25 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdb.arch/cordic.ko.bz2: New file.
* gdb.arch/cordic.ko.debug.bz2: New file.
* gdb.arch/ppc64-symtab-cordic.exp: New file.
Comments
On Wed, 25 Feb 2015 22:04:04 +0100, Jan Kratochvil wrote:
> Valgrind on F-21 x86_64 host showed me more clear what is the problem:
s/Valgrind/ASAN/
On 02/25/2015 09:04 PM, Jan Kratochvil wrote:
> +# This test won't work properly if system debuginfo is installed.
> +gdb_test_no_output "set debug-file-directory" ""
I suspect this "" here would be for "no expected output", but
given this is gdb_test_no_output, it actually means the test has
no associated message. I think you want to remove that.
Otherwise this (patch and test) looks good to me.
Thanks,
Pedro Alves
On Thu, 26 Feb 2015 12:33:10 +0100, Pedro Alves wrote:
> On 02/25/2015 09:04 PM, Jan Kratochvil wrote:
>
> > +# This test won't work properly if system debuginfo is installed.
> > +gdb_test_no_output "set debug-file-directory" ""
>
> I suspect this "" here would be for "no expected output", but
> given this is gdb_test_no_output, it actually means the test has
> no associated message. I think you want to remove that.
I do not care if "" gets removed or not but it was intentional this way.
(1) Some people prefer no gdb.sum line for testcases which really should never
FAIL and/or which are not a subject of the testfile. I am do not belong
between these people (if I care at all), though.
(2) Here I used it as the only other testcase will be often UNTESTED (due to
no ppc64 target support compiled in) - and it looks better if there is
only one UNTESTED result than one PASS + one UNTESTED which may give
a false feeling something got tested - which it was not.
> Otherwise this (patch and test) looks good to me.
So just if "" should be there or not.
Jan
On 02/26/2015 11:41 AM, Jan Kratochvil wrote:
> On Thu, 26 Feb 2015 12:33:10 +0100, Pedro Alves wrote:
>> On 02/25/2015 09:04 PM, Jan Kratochvil wrote:
>>
>>> +# This test won't work properly if system debuginfo is installed.
>>> +gdb_test_no_output "set debug-file-directory" ""
>>
>> I suspect this "" here would be for "no expected output", but
>> given this is gdb_test_no_output, it actually means the test has
>> no associated message. I think you want to remove that.
>
> I do not care if "" gets removed or not but it was intentional this way.
>
> (1) Some people prefer no gdb.sum line for testcases which really should never
> FAIL and/or which are not a subject of the testfile. I am do not belong
> between these people (if I care at all), though.
>
> (2) Here I used it as the only other testcase will be often UNTESTED (due to
> no ppc64 target support compiled in) - and it looks better if there is
> only one UNTESTED result than one PASS + one UNTESTED which may give
> a false feeling something got tested - which it was not.
>
>
>> Otherwise this (patch and test) looks good to me.
>
> So just if "" should be there or not.
Fine with to me leave it, but please add the (2) comment above
in the test then.
Thanks,
Pedro Alves
On Thu, 26 Feb 2015 12:51:45 +0100, Pedro Alves wrote:
> Fine with to me leave it, but please add the (2) comment above
> in the test then.
Checked in:
80c570537e380c1b8e48754c0ddbce2abcde2d00
+# This test won't work properly if system debuginfo is installed.
+# Test message is suppressed by "" as otherwise we could print PASS+UNTESTED
+# result to gdb.sum making a false feeling the issue has been tested.
+gdb_test_no_output "set debug-file-directory" ""
Thanks,
Jan
On 02/25/2015 02:04 PM, Jan Kratochvil wrote:
> Hi Martin,
>
> [ppc64] SEGV in ppc64_elf_get_synthetic_symtab reading a separate debug file
> https://sourceware.org/bugzilla/show_bug.cgi?id=17968
>
> Martin Sebor 2015-02-16 00:42:58 CET
> # The problem appears to be due to the change to gdb/elfread.c introduced in the commit below:
> # # commit 63524580f8372e38a6a62fd875a4252068c31150
> # # Author: Jan Kratochvil <jan.kratochvil@redhat.com>
> # # Date: Sun Apr 17 18:38:46 2011 +0000
> # # gdb/
> # # Fix convert_code_addr_to_desc_addr for ppc64 files after eu-strip.
> # # * elfread.c (elf_symfile_read): New variable synth_abfd, pass it to
> # # bfd_get_synthetic_symtab.
>
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=63524580f8372e38a6a62fd875a4252068c31150
>
> I find the patch of mine above right even for object files and your patch only
> avoided hitting the stale memory reference by disabling proper .opd parsing.
I admit I suspected that my fix only worked around the problem
without removing the root cause. Glad to know you found a more
appropriate solution.
Martin
@@ -1144,8 +1144,10 @@ elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
if (storage_needed > 0)
{
- symbol_table = (asymbol **) xmalloc (storage_needed);
- make_cleanup (xfree, symbol_table);
+ /* Memory gets permanently referenced from ABFD after
+ bfd_canonicalize_symtab so it must not get freed before ABFD gets. */
+
+ symbol_table = bfd_alloc (abfd, storage_needed);
symcount = bfd_canonicalize_symtab (objfile->obfd, symbol_table);
if (symcount < 0)
new file mode 100644
GIT binary patch
literal 2208
zcmV;R2w(R?T4*^jL0KkKS*&srrvL?||NsC0|NsC0|Nj5~|NsC0|Ns5}`~TPf{r>NL
z|M&O*{=3iy7`zRzT{*RJ$_5=MY9NATJv~w6%6O-gZA_Xn4XE@$$Yj%MWY7S}2Gr4r
z0BC61gG~&9h-CDEpfu5-$)Toz3{OzeqehwxLrfskO$eh!PftW@dTFVeo{9R9(dag)
z!fB6CpQ$}SWHj=cdYWQs=`wm`g8+>*!T>g)0QCS5Q${09i~=$kh9gWsXvEV^05m;8
zq6C;GO#skSDeW|-Cz5HWsCrLR5CGF75$bw|O)>!WJwP%vGf|-R0MG%D44O348k<PX
zKy6I`8UO~38UQrW0000027mwn27t%_00000007VcG-v<-000000BC3p001&*1c@Oj
z=%<=Ym{VmvO$LB%Mrv(C#XTpe(9qGL>KYmWrqpN|8V^Xw01s0kplASS0MG%T0B8UJ
z0009cSM{HpRWnAvc)7>A1fvBIIfjc$5&b$19k-0qsJ2^U0?y{?l^LzbHro4n)RqK#
zO8}Ym4m|_Ju%LnNp$2Anq*le45RkO^TD-8UUgd8}JFTBI%S&BLb6<o?;%%+W3F>La
zqjBtYtMS{oLX*##<3f~=ZaB#>%lb9Sd0js4KJ%hMyq~1^7*=Q!9Hb^EJYfI?Mp6;V
zyS!_6ngGsfkbt<_>dt>fIFL!UB&fdDoDNs4%rk>_S6kL!FISGGkgTfcY-XKaRHSOZ
zS3!y2E4_>YtV-(Y0|x#J{WJk_L?BCoWRjE!h=_;=!wH}zD2RX%+kOkGln5`NfenCM
zu^7f;nYVD7d%3#iy+*RlV6(C^Ty(Q-Z!dmnBO2A=ub5L!*|dNySR?GTh-g==5EZOQ
z)3htLkx8s}E)uLe7)u{#Yki{9Wo1T>M%y}u;Spb%NzQj!u5HL)PYDQ6TMRhWE#)#S
zWKZK)yBQAGIUyul+j8vX4<K+-J;B0|8UZy;OO7mIP9WBD@O1TdAJwlevXJW|)MRVS
z?CgqS!m1sc79%4p35~-jh~+1P4TTb9d=w=Cq_9GSkWRpD6gXiTim4Ka6xQS=8AvNL
zv4tv9q9c%Go_U?S$yY;RN`v>8_}2i@X6cOtFMZ?u>kZ!_Mt=}0i&tuqv9U^15;G@q
zWalxrG=vsi<O;!vB=sa_CB;jL?Pe}D9+p;RXzQ8D*uu`U0#lDauTCZW_jLR@j*qR=
zOL4=2AH(}vTA>3#5d88x2%?bl!m5_^Yn~Y0=2cgwC0`>8ia-E*3cZ|cCQ<rvCvOva
zL%JXFgz?8Dff+31MTqAeu3l8neBcOU<0E6qhnAu=PZ{%15`WjQT$4EA)+d{L<h`<2
zW?)N~pk7Cso8hv#2Pw!NInR-9vvR3`M!3PfDBGKvPv1Qnk!@siiiu|3sO_!!+|H#Q
zBMhZVNmE7v76j08BA6D@7zhkJwrQ|&!aZLDWKU>o;9Wzo`=ZgXU59zuwwH{@TIZc=
zR-#iLy~c9$I9#p`{-z9_I`@$|T21umhNGOZYGwe#uiiELiJSTH0Lw0ZOK5|_N8sbR
zEF4p!;cx6O@ddWx-bgSe^RU|UM=36>l?lYbHq4^<%}b92CX}$&v&|aR=75$<(Gu=^
znY%ZQ0M8-f+IA7B1WxA^k<8#C6ECo<`p-@7vG%7v9erdjOI4P16FLcFfIMvQ)POH)
zA$jZ7C`y6-OF^_LH3mL0h=@#_2ByocNsM%awaOX??P^kv4nJmqlj<ZzZ9<=%jl}MX
zA(lO*X5!(Y&H*36Am{!O>1K!VEo?VL*REv+8Qd>FJ#%T~Y?fDoX;TwsfsLY;XbMVw
z4rX$6rWB={B@||*%S{*iHkZG+x4pf+EC1e$EPdD|29)kJ2!R~w)fU#tWM_4`JXyxu
zTF-A;V~j=PMy*osSsteq1O|D5Qmrb~s!UgMSd>l;o~%R^;SG;zkmBHf<%(=l$`qV3
z&dlM2h%~TZu-wTyQrbmItXMS)0`%Grz=9IVu(56^yn+l+O1~EA;UaKN0Qo?_La7z|
zDSK&9J3*yP!=zSCS6@MEPdQjq8&nge@(_P}7v=2Hf39p>3@Qsy7aIC@SY`=nBCAAX
zOp>ogqt8yn!8|TewqE*gfcL-5LxyA6F$@TpPs}A4mjeldhFD=7-?F_r&skiA8D$Ku
zoFLJZ6oxTmLQZb1cU<?MB6-gepQ4Bdt|VrXMg3oJ9$V!woyLTb3kpI47YJxX$cvU~
zDFni1KFm>je5{&=+NOsR5TN!XMne|!sIX;;gDeTxv<b?_b5MjEh@AtWP`DeQ7Qr|N
zO?~o<kDV;1kbN>_2DbNmeKd+Q0ubwXTHUqZVt&;eAMVOeS`ioz<kL);@AaOKRal=t
zC3MO2PCRJKAZU9fs~6DQbZ9EDpzC1hku=gFE|2b!5^5zELL)@hxn(~+%l$@L6K^>`
z$vq0jZp!^<p<aI%l>WiAwd$HnG~${w-n;J;h``*#t(>y+x&@e~e*RixEWQ?;?b{hY
z3tq$3XXUnG^4<69+i1gnSUTw0kp)Kf=%XoTFJ=C7tKG5b^nI0Mi}VxPQ>-<Q%oZ-t
zFft%OJEMz8&XMOgY?hMkz%^7BCWg6;Yy?W3%`@|Azw3?l4m&WdlHU*P;^6XNIN$&{
z;O9G%1(AT{sAy7|;%H}Xy8LWZ`%ASAj>d(O6LU+68YCf+9Re+&Dw_815H&s&9))ag
z#@<TzO14EUyHL7|HwSTi`*wE}xfTL~@x)TUYcQiSB`q}&i!3JUlf*H;)fuXoGsyxP
z24k!s#=i(03J_DeL0QbHVI64`s>s68XBlV!Wo<ji8g^8tLqLwi#fA3&BI@}>Jk7(N
igT(b%hF-emOD7!&h)y8@P((08{x0N-aG@bs<RwmNjR|1@
literal 0
HcmV?d00001
new file mode 100644
GIT binary patch
literal 910
zcmV;919AL9T4*^jL0KkKS+>*FwEzR>fB*mg|L0w6e^yWJenh|T|6r(7V8A3uV44O{
z3PAw~Ad<iX0mTDKMj9eDPf6-}O$|Lj(WWCn01W^GKxoj=0ig7N8fY|V4F-rr@=r+g
znMvr4pe7L0Lrn}u03$R4U?3V834lyA&_RFz00000000EQ0000+00LkDNRcL*38dPd
zr>W?snHWtx(rDVD>R<r%Ov)MzBV>Sj8i9!P7=XQf_tu%wQ66ijj|>8(N-SHcU`gj?
zA?pNyWnX!os)mt37+O-nD`DLvmYG%^2d>a+K_HCG#1uk}^RA}vP{T(SZFOygwjMJK
zLm^fMDTKp*U^cT1s-c#xVLPg~xVdOMr+`#M680nhi_eOl+yxXv12QTB091wuE@UI2
z00A@E_6}Pd`!ZYj#q@4;85%NIx(FL=Kxbah82{<x(W8Yk6pMca-nO1Hxu1p?+ocOh
zqN*VzlqC=m4Z>|Cq99Qa2pepz5D4T!3ABdB&}=Z{G?Egm#i`1L{S~7#rN<L~<A!EC
z4T2Jnb~8;fm`w$O>M*AiyZ1$)V4y=GAtUE>U}KUqGkkkHTYH|qP79y5chjMBS*bGV
z^<^SbXMK@5l4xtaWhDs-P{6#yH|veP2u;*A57@v73Y4=Tl2U>MBPEc`bA&k$t7d2n
zhoP$i3Rz1<PEDmKfzwyaaL<gyan3<rrt81ZHO=`uUxkvYHG^C<bZzX@ib{t$LegR!
z-bhc2Xs|MSG~NwzDes5QZx2HVG8tlkdj#+8@31w&YMB(8g>(EC*SrfsuPvX^F-dEM
z=q?abVBmxkfMhQ?6u~pi+SeI<0Zj{|h_`NeB579>XTrwO<KU%@QNNinNKx?-6mTUc
zj{XzOhLPv6D7E*PO0C=xhb+@(sth>-t}Ln7tkQS{9T6=|0>7b?!(kHTg@6+Ylf@96
zxK+#+39rc>&`}mGUARXS-w!w#R#z`nZ82`R_$84&<y9%jTL-UqBPE6OT^0*x0nvKR
z{J9fbfq0o=*|Mo5NR{Jq?UtAhd{!8OH2^0Dweln=p^+d4u!o5ctwu>CvQ+wn#VMg^
zQyfw*r4D){23+f$10yqu2tl$W#vlrxK`=@IhJh2LC}3&NO$i_Z(Jup~Nfg-i9~kn3
krH211sj3TA8HyMfOUhbY@Ke(~!GDXnBAh5lTWRXrfWMrBLI3~&
literal 0
HcmV?d00001
new file mode 100644
@@ -0,0 +1,49 @@
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+set kobz2file ${srcdir}/${subdir}/cordic.ko.bz2
+set kofile ${objdir}/${subdir}/cordic.ko
+set kodebugbz2file ${srcdir}/${subdir}/cordic.ko.debug.bz2
+set kodebugfile ${objdir}/${subdir}/cordic.ko.debug
+
+if {[catch "system \"bzip2 -dc ${kobz2file} >${kofile}\""] != 0} {
+ untested "failed bzip2 for ${kobz2file}"
+ return -1
+}
+if {[catch "system \"bzip2 -dc ${kodebugbz2file} >${kodebugfile}\""] != 0} {
+ untested "failed bzip2 for ${kodebugbz2file}"
+ return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# This test won't work properly if system debuginfo is installed.
+gdb_test_no_output "set debug-file-directory" ""
+
+gdb_load ${kofile}
+
+set test "show architecture"
+gdb_test_multiple $test $test {
+ -re "\r\nThe target architecture is set automatically \\(currently powerpc:common64\\)\r\n$gdb_prompt $" {
+ pass $test
+ }
+ -re "\r\nThe target architecture is set automatically \\(currently .*\\)\r\n$gdb_prompt $" {
+ untested "powerpc:common64 is not supported"
+ }
+}