This is an archive of the discontinued LLVM Phabricator instance.

Differential D44285

Fix computeSymbolSizes SEGFAULT on invalid file
ClosedPublic

Authored by palmtenor on Mar 8 2018, 7:29 PM.

Details

Reviewers
dberris
kcc
dvyukov
aprantl
• espindola
jhenderson
Commits
rGbbe980dfe12d: Fix computeSymbolSizes SEGFAULT on invalid file
rL330610: Fix computeSymbolSizes SEGFAULT on invalid file
Summary

We use llvm-symbolizer in some production systems, and we run it against all possibly related files, including some that are not ELF. We noticed that for some of those invalid files, llvm-symbolizer would crash with SEGFAULT. Here is an example of such a file.

It is due to that in computeSymbolSizes, a loop uses condition

for (unsigned I = 0, N = Addresses.size() - 1; I < N; ++I) {

where if Addresses.size() is 0, N would overflow and causing the loop to access invalid memory.

Instead of patching the loop conditions, the commit makes so that the function returns early if Addresses is empty.

Validated by checking that llvm-symbolizer no longer crashes.

Diff Detail

Repository
rL LLVM

Event Timeline

palmtenor created this revision.Mar 8 2018, 7:29 PM
Herald added a subscriber: llvm-commits. · View Herald TranscriptMar 8 2018, 7:29 PM
palmtenor updated this revision to Diff 137691.Mar 8 2018, 7:32 PM
palmtenor updated this revision to Diff 137692.

Format correctly

aprantl added a comment.Mar 9 2018, 8:47 AM

Can you create a testcase for this? You should be able to hand-craft one by using a minimal assembler or yaml2obj input file and modifying it.

palmtenor added a comment.Mar 9 2018, 12:27 PM

@aprantl: The file in question is from a memory dump so a full page (2MB) large. It has zero ELF format structure... I can try see if I could get a smaller / minimal repro to test this bug. Can you point me to where the tests are?

aprantl added a comment.Mar 12 2018, 9:05 AM

Can you point me to where the tests are?

Tests for libObject are in test/Object, test/ObjectYAML, unittest/Object, unittest/ObjectYAML.

• espindola edited reviewers, added: • espindola; removed: • rafael.Mar 15 2018, 8:32 AM
palmtenor updated this revision to Diff 140025.Mar 27 2018, 5:35 PM

@aprantl Thanks for the pointing and sorry for the delay. Just added a very simple test to reproduce the crash:)

aprantl added inline comments.Mar 28 2018, 2:01 PM
test/tools/llvm-symbolizer/sym.test
22 ↗(On Diff #140025)

This will not work on all operating systems. Since the file is only 1K in size, would you mind just checking in the binary?

24 ↗(On Diff #140025)

This will also succeed if llvm-symbolizer is symlinked to /bin/true. Is there some way you could check the output?

palmtenor added inline comments.Mar 28 2018, 2:14 PM
test/tools/llvm-symbolizer/sym.test
22 ↗(On Diff #140025)

I can do that. How can I submit a binary file on Phabricator?

24 ↗(On Diff #140025)

if llvm-symbolizer is symlinked to /bin/true

It actually seem to work:

$ /bin/true -obj ~/test-bad-file  < test_input
$ echo $?
0

Just want to check the tool doesn't crash here...

palmtenor updated this revision to Diff 140149.Mar 28 2018, 3:39 PM

@aprantl Updated with the test file committed:)

aprantl accepted this revision.Mar 28 2018, 4:04 PM
aprantl added inline comments.
test/tools/llvm-symbolizer/sym.test
24 ↗(On Diff #140025)

My point was that a test that just checks the exit status without checking any output even works when the program doesn't do anything at all, so if there is any output you can check from llvm-symbolizer, that would be much better!
If it doesn't generate any output, you can use FileCheck --allow-empty.

This revision is now accepted and ready to land.Mar 28 2018, 4:04 PM
palmtenor added inline comments.Mar 28 2018, 4:14 PM
test/tools/llvm-symbolizer/sym.test
24 ↗(On Diff #140025)

It should just output the normal unknown result

??
??:0:0

Do you feel it's worth checking?

aprantl added inline comments.Mar 28 2018, 5:25 PM
test/tools/llvm-symbolizer/sym.test
24 ↗(On Diff #140025)

Yes, please do check for the result. Thanks!

palmtenor updated this revision to Diff 140340.Mar 29 2018, 2:41 PM

@aprantl Sure! Just updated the test:)

aprantl accepted this revision.Mar 29 2018, 3:19 PM

Thanks. LGTM

• espindola accepted this revision.Apr 2 2018, 12:55 PM

LGTM

It would be nice to replace the binary input with something created with yaml2obj if possible.

palmtenor added a comment.Apr 20 2018, 7:59 PM

@espindola @aprantl I don't think I have commit access to the repo. Can you help me to merge? Thanks a lot!

Closed by commit rL330610: Fix computeSymbolSizes SEGFAULT on invalid file (authored by adrian). · Explain WhyApr 23 2018, 9:13 AM
This revision was automatically updated to reflect the committed changes.
dblaikie added a subscriber: dblaikie.Oct 14 2020, 10:45 PM

Looks like this test isn't testing what's intended - the input file wasn't committed, by the looks of it, so it's just testing llvm-symbolizer attempting to symbolize an address with no input file. I found this as I'm improving llvm-symbolizer to return non-zero on various error cases (including file not found, in this instance).

@aprantl could you see if the necessary test input file could be added?

Herald added a reviewer: jhenderson. · View Herald TranscriptOct 14 2020, 10:45 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: rupprecht, MaskRay. · View Herald Transcript
aprantl added a comment.Oct 16 2020, 1:26 PM

I'm not sure if I specifically can help here: The only change I committed (on behalf of the author) was this one, which tests the echo "0x1" > %t.input case.

dblaikie added a comment.Oct 16 2020, 1:44 PM
In D44285#2335892, @aprantl wrote:

I'm not sure if I specifically can help here: The only change I committed (on behalf of the author) was this one, which tests the echo "0x1" > %t.input case.

Sorry, that's the test case I'm talking about - the test case uses an input file ("%p/Inputs/zero") that wasn't included in the patch/committed, so far as I can see? And so the test isn't testing what it is intending to test, so far as I can tell. I believe the test was meant to include a (probably valid ELF? MachO? Maybe it doesn't matter which) file no sections in it, to tickle the for (unsigned I = 0, N = Addresses.size() - 1; I < N; ++I) { loop - or to tickle the Addresses.empty() check intended to fix the issue3

jhenderson added a comment.Oct 19 2020, 3:09 AM

It looks like the input needs to have no sections or symbols, but otherwise be a valid object file. I think the following YAML would do the trick, but I haven't verified it hits the appropriate code path:

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
SectionHeaderTable:
  NoHeaders: true

Revision Contents

PathSize
llvm/
trunk/
lib/
Object/
4 lines
test/
tools/
llvm-symbolizer/
6 lines

Diff 143577

llvm/trunk/lib/Object/SymbolSize.cpp

Show First 20 Lines • Show All 60 Lines • ▼ Show 20 Lines for (symbol_iterator I = O.symbol_begin(), E = O.symbol_end(); I != E; ++I) {
++SymNum; ++SymNum;
} }
for (SectionRef Sec : O.sections()) { for (SectionRef Sec : O.sections()) {
uint64_t Address = Sec.getAddress(); uint64_t Address = Sec.getAddress();
uint64_t Size = Sec.getSize(); uint64_t Size = Sec.getSize();
Addresses.push_back( Addresses.push_back(
{O.symbol_end(), Address + Size, 0, getSectionID(O, Sec)}); {O.symbol_end(), Address + Size, 0, getSectionID(O, Sec)});
} }
if (Addresses.empty())
return Ret;
array_pod_sort(Addresses.begin(), Addresses.end(), compareAddress); array_pod_sort(Addresses.begin(), Addresses.end(), compareAddress);
// Compute the size as the gap to the next symbol // Compute the size as the gap to the next symbol
for (unsigned I = 0, N = Addresses.size() - 1; I < N; ++I) { for (unsigned I = 0, N = Addresses.size() - 1; I < N; ++I) {
auto &P = Addresses[I]; auto &P = Addresses[I];
if (P.I == O.symbol_end()) if (P.I == O.symbol_end())
continue; continue;
Show All 18 Lines

llvm/trunk/test/tools/llvm-symbolizer/sym.test

Show All 13 Lines
# int x = 1; # int x = 1;
# return inc(&x); # return inc(&x);
#} #}
# #
#Build as : clang -g -O2 addr.c #Build as : clang -g -O2 addr.c
RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
RUN: llvm-symbolizer -inlining -print-address -pretty-print -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck --check-prefix="PRETTY" %s RUN: llvm-symbolizer -inlining -print-address -pretty-print -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck --check-prefix="PRETTY" %s
RUN: echo "0x1" > %t.input
RUN: llvm-symbolizer -obj=%p/Inputs/zero < %t.input | FileCheck --check-prefix="ZERO" %s
#CHECK: some text #CHECK: some text
#CHECK: 0x40054d #CHECK: 0x40054d
#CHECK: main #CHECK: main
#CHECK: {{[/\]+}}tmp{{[/\]+}}x.c:14:0 #CHECK: {{[/\]+}}tmp{{[/\]+}}x.c:14:0
#CHECK: some text2 #CHECK: some text2
# #
#PRETTY: some text #PRETTY: some text
#PRETTY: {{[0x]+}}40054d: inctwo at {{[/\]+}}tmp{{[/\]+}}x.c:3:3 #PRETTY: {{[0x]+}}40054d: inctwo at {{[/\]+}}tmp{{[/\]+}}x.c:3:3
#PRETTY: (inlined by) inc at {{[/\]+}}tmp{{[/\]+}}x.c:7:0 #PRETTY: (inlined by) inc at {{[/\]+}}tmp{{[/\]+}}x.c:7:0
#PRETTY (inlined by) main at {{[/\]+}}tmp{{[/\]+}}x.c:14:0 #PRETTY (inlined by) main at {{[/\]+}}tmp{{[/\]+}}x.c:14:0
#PRETTY: some text2 #PRETTY: some text2
#
#ZERO: ??
#ZERO: ??:0:0