Issue 5876052: [ASan] add full strtoll implementation and interceptor (Closed)
|
Created:
12 years, 9 months ago by samsonov Modified:
12 years, 9 months ago CC:
llvm-commits_cs.uiuc.edu Base URL:
https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/asan/ Visibility:
Public. |
Patch Set 1 : [ASan] add full strtoll implementation and interceptor #Patch Set 2 : [ASan] use shorter wrapper for strtoll instead of full replacement #
Total comments: 4
Patch Set 3 : minor fixes #
MessagesTotal messages: 8
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.
Sign in to reply to this message.
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. 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, <samsonov@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/<http://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 > > >
Sign in to reply to this message.
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@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/5876...> > > > > 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 > > > > > >
Sign in to reply to this message.
On Thu, Mar 22, 2012 at 1:08 PM, <samsonov@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. 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@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/<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/<http://codereview.appspot.com/5876052/> >
Sign in to reply to this message.
On Fri, Mar 23, 2012 at 12:25 AM, Kostya Serebryany <kcc@google.com> wrote: > > > On Thu, Mar 22, 2012 at 1:08 PM, <samsonov@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. 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@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/<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/<http://codereview.appspot.com/5876052/> >> > > -- Alexey Samsonov Software Engineer, Moscow samsonov@google.com
Sign in to reply to this message.
On 2012/03/23 04:40:03, samsonov wrote: > On Fri, Mar 23, 2012 at 12:25 AM, Kostya Serebryany <mailto:kcc@google.com> wrote: > > > > > > > On Thu, Mar 22, 2012 at 1:08 PM, <mailto:samsonov@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@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/5876...> > >> > > > > > > > -- > Alexey Samsonov > Software Engineer, Moscow > mailto:samsonov@google.com
Sign in to reply to this message.
Look good! http://codereview.appspot.com/5876052/diff/5002/asan_interceptors.cc File asan_interceptors.cc (right): http://codereview.appspot.com/5876052/diff/5002/asan_interceptors.cc#newcode72 asan_interceptors.cc:72: # if !defined(_WIN32) use ASAN_CAN_USE_STRTOLL http://codereview.appspot.com/5876052/diff/5002/asan_interceptors.cc#newcode657 asan_interceptors.cc:657: } else { no "else" after return.
Sign in to reply to this message.
r153376, thanks! http://codereview.appspot.com/5876052/diff/5002/asan_interceptors.cc File asan_interceptors.cc (right): http://codereview.appspot.com/5876052/diff/5002/asan_interceptors.cc#newcode72 asan_interceptors.cc:72: # if !defined(_WIN32) On 2012/03/23 15:35:45, kcc1 wrote: > use ASAN_CAN_USE_STRTOLL ASAN_INTERCEPT_STRTOLL, if you don't mind. http://codereview.appspot.com/5876052/diff/5002/asan_interceptors.cc#newcode657 asan_interceptors.cc:657: } else { On 2012/03/23 15:35:45, kcc1 wrote: > no "else" after return. Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
