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

[samsonov at google.com]

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?


> 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/%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/