Bug 26016 - Bad mmap error check in coredump-filter.c / testsuite
Summary: Bad mmap error check in coredump-filter.c / testsuite
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: testsuite (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-20 13:10 UTC by Hanno Boeck
Modified: 2020-05-20 14:55 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2020-05-20 00:00:00

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Hanno Boeck 2020-05-20 13:10:27 UTC 
In coredump-filter.c [1] there is this code:

  void *ret = mmap (addr, size, prot, flags, fd, offset);

  assert (ret != NULL);

The mmap function never returns NULL, on errors it returns MAP_FAILED (or -1). Thus this check is wrong and should probably be "ret != MAP_FAILED".

(Sidenote: asserts usually shouldn't be used for error checking, but this may be a design decision due to this being a test suite.)

[1] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/testsuite/gdb.base/coredump-filter.c;h=f53a933a72545741094fc9549ff4411e3741adbe;hb=HEAD
Comment 1 Simon Marchi 2020-05-20 14:41:45 UTC 
Thanks for the report.  Was this found using some static analysis tool, or you just stumbled on it?

I'll also fix the spots that check against -1 to use MAP_FAILED.
Comment 2 Simon Marchi 2020-05-20 14:42:30 UTC 
(In reply to Hanno Boeck from comment #0)
> (Sidenote: asserts usually shouldn't be used for error checking, but this
> may be a design decision due to this being a test suite.)

For test programs I think it's fine.
Comment 3 cvs-commit@gcc.gnu.org 2020-05-20 14:51:30 UTC 
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=41977d16e4ee5b9ad01abf2cfce6edbfb6d79541

commit 41977d16e4ee5b9ad01abf2cfce6edbfb6d79541
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Wed May 20 10:50:39 2020 -0400

    gdb/testsuite: check mmap ret val against MAP_FAILED
    
    Fixup a few spots in the testsuite that use mmap to consistently check
    the return value against MAP_FAILED.
    
    One spot in gdb.base/coredump-filter.c checked against NULL, that is
    wrong.  The other spots either didn't check, or checked against -1.
    MAP_FAILED has the value -1, at least on Linux, but it's better to check
    against the documented define.
    
    gdb/testsuite/ChangeLog:
    
            PR gdb/26016
            * gdb.base/coredump-filter.c (do_mmap): Check mmap ret val
            against MAP_FAILED.
            * gdb.base/coremaker.c (mmapdata): Likewise.
            * gdb.base/jit-reader-host.c (main): Likewise.
            * gdb.base/sym-file-loader.c (load): Likewise.
            (load_shlib): Likewise.
Comment 4 Hanno Boeck 2020-05-20 14:54:30 UTC 
(In reply to Simon Marchi from comment #1)
> Thanks for the report.  Was this found using some static analysis tool, or
> you just stumbled on it?

So this is a very good question and the answer may surprise you :-)

I learned about this type of bug and I thought "I can write a trivial shellscript to find these kinds of bugs". So if my 6 lines of bash count as a static analysis tool then yes.

I plan to release it soon, I'll post a link.
Comment 5 Simon Marchi 2020-05-20 14:55:41 UTC 
Well, it worked well enough to find one bug, thanks!