[llvm-commits] [ASan] add full strtoll implementation and interceptor (issue 5876052)

[samsonov at google.com]

On 2012/03/23 04:40:03, samsonov wrote:
> On Fri, Mar 23, 2012 at 12:25 AM, Kostya Serebryany
<mailto:kcc at google.com> wrote:

> >
> >
> > On Thu, Mar 22, 2012 at 1:08 PM, <mailto:samsonov at google.com> wrote:
> >
> >> On 2012/03/22 16:15:11, kcc1 wrote:
> >>
> >>> I support the intention but I don't like the approach.
> >>> You effectively reimplemented strtoll, which may appear to be even
> >>>
> >> more
> >>
> >>> complex function than it looks.
> >>> Now we suddenly need to have an exhaustive test for this part of
libc,
> >>> including messy errno business.
> >>> Meanwhile, why can't you use the original strtoll from libc?
> >>> it gives you the endptr which could be used by asan run-time to
detect
> >>>
> >> the
> >>
> >>> right bound of access.
> >>>
> >>
> >> I'd also prefer to use libc strtoll, but we can't make our checks
> >> complete in this case:
> >> according to specification, strtoll sets endptr to the first char
that
> >> is not a part of the number OR the first char of the string if no
digits
> >> are found. So, if a string consists of just 100 whitespaces, endptr
will
> >> point to its beginning, while the implementation actually had to
look
> >> 100 symbols ahead. We can:
> >> (1) don't handle this case (i.e. miss possible error reports) - in
> >> general, this is not very good, as I think that memory errors are
more
> >> likely to occur in cases when there is no number somewhy.
> >> (2) add some hacks to handle this case separately
> >> (3) re-implement the whole thing.
> >>
> >> I'm not quite happy with the result of (3) either, but think we can
get
> >> away with this.
> >
> >
> >
> >
> >> Or are you afraid of maintenance cost of this code?
> >
> >
> > Exactly.
> > I'd prefer to figure out the "last accessed byte" separately, based
on the
> > input and/or endptr.
> >

> OK, I see what you mean and will implement this.

Done.


> It is fine if we report an un-addressable access after the strtoll
call,
> > not before.
> >


> >
> >


> >
> > --kcc
> >
> >
> >>
> >>
> >>
> >>  Also, instead of ifndef(_WIN32) I would prefer more meaningful
guards
> >>>
> >> like
> >>
> >>> #ifdef(ASAN_CAN_USE_STRTOLL), defined in one place.
> >>>
> >>
> >>  --kcc
> >>>
> >>
> >>  On Thu, Mar 22, 2012 at 5:17 AM, <mailto:samsonov at google.com>
wrote:
> >>>
> >>
> >>  > Reviewers: Evgeniy Stepanov, kcc1, timurrrr_at_google_com,
> >>> >
> >>> > Message:
> >>> > We need to intercept strtol/atoi functions - calling them may
result
> >>>
> >> in
> >>
> >>> > OOB reads (and they appear in stack traces of Chromium crash
> >>>
> >> reports).
> >>
> >>> > Looks like we have to re-implement them. Here's the major step
to do
> >>> > this.
> >>> >
> >>> >
> >>> >
> >>> > Please review this at
> >>>
> >>
> >> http://codereview.appspot.com/****5876052/%253Chttp://**
> >>

codereview.appspot.com/**5876052/<http://codereview.appspot.com/**5876052/%3Chttp://codereview.appspot.com/5876052/>
> >> >
> >>
> >>  >
> >>> > Affected files:
> >>> >  M     asan_interceptors.cc
> >>> >  M     asan_interceptors.h
> >>> >  M     asan_internal.h
> >>> >  M     asan_posix.cc
> >>> >  M     asan_rtl.cc
> >>> >  M     asan_win.cc
> >>> >  M     tests/asan_test.cc
> >>> >
> >>> >
> >>> >
> >>>
> >>
> >>
> >>
> >>

http://codereview.appspot.com/**5876052/%3Chttp://codereview.appspot.com/5876052/>
> >>
> >
> >


> --
> Alexey Samsonov
> Software Engineer, Moscow
> mailto:samsonov at google.com



http://codereview.appspot.com/5876052/