args: Fix segfaults when reading string arguments #1480
Conversation
| * f7db5000-f7dd2000 r--p 00000000 fd:00 10701229 /lib/i386-linux-gnu/libc-2.31.so | ||
| */ | ||
| if (strlen(buf) < 74) | ||
| continue; |
There was a problem hiding this comment.
Is this really needed? I think it's fragile and unnecessary as it also checks the permission.
There was a problem hiding this comment.
Without checking this unnamed memory regions, I see hermes run still crashed because of dereferencing those regions.
There was a problem hiding this comment.
But I think unnamed (anon) regions can be legal and contain valid strings.
There was a problem hiding this comment.
I initially checked only the read permission as follows.
diff --git a/libmcount/record.c b/libmcount/record.c
index 77a96742..292733f3 100644
--- a/libmcount/record.c
+++ b/libmcount/record.c
@@ -318,6 +318,10 @@ static void update_mem_regions(struct mcount_mem_regions *regions)
p = next + 1;
end = strtoul(p, &next, 16);
+ p = next + 1;
+ if (*p != 'r')
+ continue;
+
if (strstr(next, "[heap]")) {
end = ROUND_UP(end, HEAP_REGION_UNIT);
if (end > regions->brk)But I also saw another segfault.
$ uftrace record -P. -a ./bin/hermes
WARN: Segmentation fault: invalid permission (addr: 0x331e93800000)
WARN: if this happens only with uftrace, please consider -e/--estimate-return option.
WARN: Backtrace from uftrace v0.12-53-g1ee2 ( x86_64 dwarf python luajit tui perf sched dynamic )
WARN: =====================================
WARN: [14] (hermes::uninitializedCopy[6107db] <= hermes::vm::DynamicStringPrimitive::DynamicStringPrimitive[60cda1])
WARN: [13] (hermes::vm::DynamicStringPrimitive::DynamicStringPrimitive[60cd2f] <= hermes::vm::GCBase::constructCell[614324])
WARN: [12] (hermes::vm::GCBase::constructCell[6142c1] <= hermes::vm::HadesGC::makeA[6138a8])
WARN: [11] (hermes::vm::HadesGC::makeA[6137e9] <= hermes::vm::GCBase::makeA[612652])
WARN: [10] (hermes::vm::GCBase::makeA[6125c1] <= hermes::vm::GCBase::makeAVariable[61139d])
WARN: [9] (hermes::vm::GCBase::makeAVariable[611305] <= hermes::vm::Runtime::makeAVariable[610333])
WARN: [8] (hermes::vm::Runtime::makeAVariable[6102ed] <= hermes::vm::DynamicStringPrimitive::createLongLived[60cf1a])
WARN: [7] (hermes::vm::DynamicStringPrimitive::createLongLived[60ce8e] <= hermes::vm::StringPrimitive::createLongLived[5bef94])
WARN: [6] (hermes::vm::StringPrimitive::createLongLived[5bef2b] <= hermes::vm::Runtime::allocateCharacterString[5c92ca])
WARN: [5] (hermes::vm::Runtime::allocateCharacterString[5c9257] <= hermes::vm::Runtime::initCharacterStrings[5c91d1])
WARN: [4] (hermes::vm::Runtime::initCharacterStrings[5c90f7] <= hermes::vm::Runtime::Runtime[5c1486])
WARN: [3] (hermes::vm::Runtime::Runtime[5c019f] <= hermes::vm::Runtime::create[5bf77e])
WARN: [2] (hermes::vm::Runtime::create[5bf5e7] <= repl[417b10])
WARN: [1] (repl[417ad6] <= main[40bab8])
WARN: [0] (main[40b9ae] <= __libc_start_main[7f75cc3980b3])
Please report this bug to https://github.com/namhyung/uftrace/issues.
WARN: child terminated by signal: 11: Segmentation fault
With some debugging, I found that one of unnamed region make the crash as follows.
00400000-0102d000 r-xp 00000000 fd:02 1793899 /home/honggyu/work/hermes/build/bin/hermes
0122c000-01235000 r--p 00c2c000 fd:02 1793899 /home/honggyu/work/hermes/build/bin/hermes
01235000-01239000 rw-p 00c35000 fd:02 1793899 /home/honggyu/work/hermes/build/bin/hermes
01239000-07d19000 rw-p 00000000 00:00 0 [heap]
52d5f000000-52d5f414000 rw-p 00000000 00:00 0
52d5f414000-52d5f415000 ---p 00000000 00:00 0
52d5f415000-52d5f814000 rw-p 00000000 00:00 0
52d5f814000-52d5f815000 ---p 00000000 00:00 0
52d5f815000-52d5fc00000 rw-p 00000000 00:00 0 <--- This region access makes the crash
52d5fc00000-52da0000000 ---p 00000000 00:00 0
I don't have a clear idea but the crashed region has a valid read permission but got crashed. I think the chances of accessing valid strings in unnamed regions are relatively low so wanted to avoid possible crashes.
There was a problem hiding this comment.
Unnamed regions can be (part of) heap so we cannot just skip. It'd be nice if we can debug what's the real issue.
In your above example, the address doesn't belong to the region.
There was a problem hiding this comment.
In my case, after checking the read permission I'm seeing another failure.
$ uftrace record -P. -a bin/hermes
hermes: hermes/lib/VM/gcs/HadesGC.cpp:3041: void hermes::vm::HadesGC::OldGen::setTargetSizeBytes(size_t): Assertion `!targetSizeBytes_ && "Should only initialise targetSizeBytes_ once."' failed.
WARN: child terminated by signal: 6: Aborted
There was a problem hiding this comment.
And checking the string length as your original patch doesn't work too.
There was a problem hiding this comment.
Hmm.. That could be a different issue then.
There was a problem hiding this comment.
Ok, let's fix the permission check alone for now. It's an obvious bug.
|
@namhyung @honggyukim Just sending a ping on this. It has a git push which has no comment (maybe this PR can be revived) |
|
Note that the permission check is merged already. The remaining part is the string length check which is arbitrary and I don't understand why it matters. |
|
@honggyukim maybe we can close this for now if you don't work on it anymore. |
|
I have tested the same program, but it still gets crashed in 82bc451b64e4757f4816ebbc54d66c7fb5df7d30 and can be fixed with this PR. This approach might be fragile but we better keep it open until we find the solid fix. |
In some cases, argument read for string could make the target program get crashed. The following sequence shows the problem. $ git clone [email protected]:facebook/hermes.git && cd hermes $ mkdir build && cd build $ cmake .. $ make -j $ uftrace record -P. -a ./bin/hermes WARN: Segmentation fault: invalid permission (addr: 0x29f1f1400000) ... WARN: child terminated by signal: 11: Segmentation fault This crash happens becasue of string dereference in some regions. Some memory regions do have read permission but still make the target program crashed while string dereference. To avoid this problem, we better skip memory regions, which don't have names of them. Fixed: namhyung#1477 Signed-off-by: Honggyu Kim <[email protected]>
honggyukim
force-pushed
the
check/fix-str-arg-segfault
branch
from
7f5c7b1 to
ac67726
Compare
|
I've just updated this PR only dropping already merged part. |
In some cases, argument read for string could make the target program
get crashed. The following sequence shows the problem.
This crash happens becasue of string dereference for non readable memory
regions.
uftrace provides check_mem_region() for this purpose, but it doesn't
check whether each memory region has read permission or not.
In addition, some memory regions do have read permission but still make
the target program crashed. To avoid this problem, we better skip
memory regions, which don't have names of them.
Fixed: #1477
Signed-off-by: Honggyu Kim [email protected]