Skip to content

Commit

Permalink
Make sure brk doesn't clobber existing mappings
Browse files Browse the repository at this point in the history
This was causing the random node segfaults. Node does manual ASLR, so it
would randomly pick an address for a memory mapping near the brk region,
then later something would raise the brk and clobber that mapping,
replacing it with zeroes. Shortly afterward something would crash on a
null pointer.

Took a lot of digging around in rr to find this. The crash is never
directly connected to the brk call, of course. I set a watchpoint on the
pointer that got clobbered and saw it had been zero since the page was
mapped, but it took setting a watchpoint on the pointer to that pointer
to find that when it was mapped before that and was valid at that point.

At this point I'm ready to say node is fixed! #90
  • Loading branch information
tbodt committed Jul 3, 2020
1 parent 2a37495 commit 6f6c26d
Showing 1 changed file with 14 additions and 14 deletions.
28 changes: 14 additions & 14 deletions kernel/mmap.c
Original file line number Diff line number Diff line change
Expand Up @@ -213,24 +213,22 @@ addr_t sys_brk(addr_t new_brk) {
STRACE("brk(0x%x)", new_brk);
struct mm *mm = current->mm;

if (new_brk != 0 && new_brk < mm->start_brk)
return _EINVAL;
write_wrlock(&mm->mem.lock);
if (new_brk < mm->start_brk)
goto out;
addr_t old_brk = mm->brk;
if (new_brk == 0) {
write_wrunlock(&mm->mem.lock);
return old_brk;
}
// TODO check for not going too high

if (new_brk > old_brk) {
// expand heap: map region from old_brk to new_brk
int err = pt_map_nothing(&mm->mem, PAGE_ROUND_UP(old_brk),
PAGE_ROUND_UP(new_brk) - PAGE_ROUND_UP(old_brk), P_WRITE);
if (err < 0) {
write_wrunlock(&mm->mem.lock);
return err;
}
// round up because of the definition of brk: "the first location after the end of the uninitialized data segment." (brk(2))
// if the brk is 0x2000, page 0x2000 shouldn't be mapped, but it should be if the brk is 0x2001.
page_t start = PAGE_ROUND_UP(old_brk);
pages_t size = PAGE_ROUND_UP(new_brk) - PAGE_ROUND_UP(old_brk);
if (!pt_is_hole(&mm->mem, start, size))
goto out;
int err = pt_map_nothing(&mm->mem, start, size, P_WRITE);
if (err < 0)
goto out;
} else if (new_brk < old_brk) {
// shrink heap: unmap region from new_brk to old_brk
// first page to unmap is PAGE(new_brk)
Expand All @@ -239,6 +237,8 @@ addr_t sys_brk(addr_t new_brk) {
}

mm->brk = new_brk;
out:;
addr_t brk = mm->brk;
write_wrunlock(&mm->mem.lock);
return new_brk;
return brk;
}

0 comments on commit 6f6c26d

Please sign in to comment.