<br><br><div class="gmail_quote">On Fri, Mar 23, 2012 at 12:25 AM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com">kcc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div class="gmail_quote"><div><div class="h5">On Thu, Mar 22, 2012 at 1:08 PM, <span dir="ltr"><<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On 2012/03/22 16:15:11, kcc1 wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I support the intention but I don't like the approach.<br>
You effectively reimplemented strtoll, which may appear to be even<br>
</blockquote>
more<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
complex function than it looks.<br>
Now we suddenly need to have an exhaustive test for this part of libc,<br>
including messy errno business.<br>
Meanwhile, why can't you use the original strtoll from libc?<br>
it gives you the endptr which could be used by asan run-time to detect<br>
</blockquote>
the<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
right bound of access.<br>
</blockquote>
<br></div>
I'd also prefer to use libc strtoll, but we can't make our checks<br>
complete in this case:<br>
according to specification, strtoll sets endptr to the first char that<br>
is not a part of the number OR the first char of the string if no digits<br>
are found. So, if a string consists of just 100 whitespaces, endptr will<br>
point to its beginning, while the implementation actually had to look<br>
100 symbols ahead. We can:<br>
(1) don't handle this case (i.e. miss possible error reports) - in<br>
general, this is not very good, as I think that memory errors are more<br>
likely to occur in cases when there is no number somewhy.<br>
(2) add some hacks to handle this case separately<br>
(3) re-implement the whole thing.<br>
<br>
I'm not quite happy with the result of (3) either, but think we can get<br>
away with this. </blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Or are you afraid of maintenance cost of this code?</blockquote>
<div><br></div></div></div><div>Exactly. </div><div>I'd prefer to figure out the "last accessed byte" separately, based on the input and/or endptr. </div></div></blockquote><div><br></div><div>OK, I see what you mean and will implement this.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>It is fine if we report an un-addressable access after the strtoll call, not before. </div>
</div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div> </div></div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote">
<div><br></div><div>--kcc </div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, instead of ifndef(_WIN32) I would prefer more meaningful guards<br>
</blockquote>
like<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
#ifdef(ASAN_CAN_USE_STRTOLL), defined in one place.<br>
</blockquote>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
--kcc<br>
</blockquote>
<br>
</div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Thu, Mar 22, 2012 at 5:17 AM, <mailto:<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>> wrote:<br>
</blockquote>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Reviewers: Evgeniy Stepanov, kcc1, timurrrr_at_google_com,<br>
><br>
> Message:<br>
> We need to intercept strtol/atoi functions - calling them may result<br>
</blockquote>
in<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> OOB reads (and they appear in stack traces of Chromium crash<br>
</blockquote>
reports).<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Looks like we have to re-implement them. Here's the major step to do<br>
> this.<br>
><br>
><br>
><br>
> Please review this at<br>
</blockquote>
<br>
</div><a href="http://codereview.appspot.com/**5876052/%3Chttp://codereview.appspot.com/5876052/" target="_blank">http://codereview.appspot.com/<u></u>**5876052/%3Chttp://<u></u>codereview.appspot.com/<u></u>5876052/</a>><div>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> Affected files:<br>
> M asan_interceptors.cc<br>
> M asan_interceptors.h<br>
> M asan_internal.h<br>
> M asan_posix.cc<br>
> M asan_rtl.cc<br>
> M asan_win.cc<br>
> M tests/asan_test.cc<br>
><br>
><br>
><br>
</blockquote>
<br>
<br>
<br>
</div><a href="http://codereview.appspot.com/5876052/" target="_blank">http://codereview.appspot.com/<u></u>5876052/</a><br>
</blockquote></div></div><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov</div><div>Software Engineer, Moscow</div><div><a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a></div><br>