[master/7.12] Fix heap-buffer-overflow in explicit_location_lex_one
Commit Message
I build GDB with -fsanitize=address, and see the error in tests,
(gdb) PASS: gdb.linespec/ls-errs.exp: lang=C++: break 3 foo
break -line 3 foo^M
Comments
On 08/12/2016 05:44 AM, Yao Qi wrote:
> the code in question is:
>
>> /* Special case: C++ operator,. */
>> if (language->la_language == language_cplus
>> && strncmp (*inp, "operator", 8) <--- [1]
>> && (*inp)[9] == ',')
>> (*inp) += 9;
>> ++(*inp);
[snip]
Yeah, that does look odd... Normally, I would not leave the result of
strcmp et al unchecked like this since those functions do not return a
boolean value. So I'd say it must have been a bad day. A really bad day.
> Is it OK?
While your patch alters the behavior ever so slightly (originally, it
was meant to skip over "operator," -- now it skips over "operator" and
the next char), I don't think that it will impact what this code
actually does. It looks good to me.
Good catch!
Keith
On Fri, Aug 12, 2016 at 5:39 PM, Keith Seitz <keiths@redhat.com> wrote:
> While your patch alters the behavior ever so slightly (originally, it
> was meant to skip over "operator," -- now it skips over "operator" and
> the next char), I don't think that it will impact what this code
> actually does. It looks good to me.
>
Thanks for the review, Keith. Patch is pushed into master and 7.12.
=================================================================^M
==4401==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000047487 at pc 0x819d8e bp 0x7fff4e4e6bb0 sp 0x7fff4e4e6ba8^M
READ of size 1 at 0x603000047487 thread T0^[[1m^[[0m^M
#0 0x819d8d in explicit_location_lex_one /home/yao/SourceCode/gnu/gdb/git/gdb/location.c:502^M
#1 0x81a185 in string_to_explicit_location(char const**, language_defn const*, int) /home/yao/SourceCode/gnu/gdb/git/gdb/location.c:556^M
#2 0x81ac10 in string_to_event_location(char**, language_defn const*) /home/yao/SourceCode/gnu/gdb/git/gdb/location.c:687^
the code in question is:
> /* Special case: C++ operator,. */
> if (language->la_language == language_cplus
> && strncmp (*inp, "operator", 8) <--- [1]
> && (*inp)[9] == ',')
> (*inp) += 9;
> ++(*inp);
The error is caused by the access to (*inp)[9] if 9 is out of its bounds.
However [1] looks odd to me, because if strncmp returns true (non-zero),
the following check "(*inp)[9] == ','" makes no sense any more. I
suspect it was a typo in the code we meant to "strncmp () == 0". Another
problem in the code above is that if *inp is "operator,", we first
increment *inp by 9, and then increment it by one again, which is wrong
to me. We should only increment *inp by 8 to skip "operator", and go
back to the loop header to decide where we stop.
Is it OK?
gdb:
2016-08-11 Yao Qi <yao.qi@linaro.org>
* location.c (explicit_location_lex_one): Compare the return
value of strncmp with zero. Don't check (*inp)[9]. Increment
*inp by 8.
---
gdb/location.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
@@ -498,9 +498,8 @@ explicit_location_lex_one (const char **inp,
{
/* Special case: C++ operator,. */
if (language->la_language == language_cplus
- && strncmp (*inp, "operator", 8)
- && (*inp)[9] == ',')
- (*inp) += 9;
+ && strncmp (*inp, "operator", 8) == 0)
+ (*inp) += 8;
++(*inp);
}
}