[llvm-commits] [ASan] The first version of the RTL for Windows (issue 5647052)

[timurrrr at google.com]

PTAL


On Wed, Feb 8, 2012 at 7:30 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:
Aaron,
can you please make the comments in the web interface of rietveld?
(by double-clicking the source line you'd like to comment on)
It's a bit more convenient to see all the comments in the web diff than
having a half there and another half in the email.
Thanks!

>> +// Disable "getenv and strcpy are unsafe" warnings.
>> +#pragma warning(disable: 4996)
> Instead of the pragma to disable warnings, why not just fix getenv and
strcpy?
Turns out I've forgotten to remove "strcpy" from this comment;
and now I've got rid of getenv() too.

>> +void *AsanMprotect(uintptr_t fixed_addr, size_t size) {
>> +  return VirtualAlloc((LPVOID)fixed_addr, size,
>> +                      MEM_RESERVE | MEM_COMMIT, PAGE_NOACCESS);
>> +}
> Shouldn't this be doing a VirtualProtect instead of allocating?
It turns out AsanMprotect must allocate + protect, so VirtualProtect is
not enough.
I don't like the definition of AsanMprotect too :)

> The stack will always be a single block, but I think what you're
> getting there is including the stack's guard page.  Are you sure you
> want that?  Also, I think that's only getting you the reserved size
> and not the committed size.
That's a good question.
I'm not sure. I've added a comment so I'll definitely come back to this
question once I start writing tests for stack OOBs.

> Since you're already using DbgHelp APIs, would StackWalk64 make a bit
> more sense?  IIRC, it's a bit more accurate than
> CaptureStackBackTrace.  Certainly it gives you more control.

> You may want to look at LLVMUnhandledExceptionFilter in Signals.inc
> because a lot of the stack walking fun is already done there.  Perhaps
> we could refactor it to use common code?
I'll decide later.
First we need to be sure that CaptureStackBackTrace is not what we want.

--
Thanks,
Timur


http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_interceptors.cc
File lib/asan/asan_interceptors.cc (right):

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_interceptors.cc#newcode32
lib/asan/asan_interceptors.cc:32: # include <intrin.h>  // FIXME: remove
when we start intercepting on Windows.
On 2012/02/08 15:40:33, glider wrote:
> Do you still need it after you've declared _ReturnAddress?
Yes, intrinsic versions of memcpy/memset are defined there.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc
File lib/asan/asan_win.cc (right):

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode59
lib/asan/asan_win.cc:59: CHECK(fd == 2);  // Looks like we only use
stderr for AsanWrite?
On 2012/02/08 15:30:09, glider wrote:
> I think we may want to use other fds someday (I'm considering output
redirection
> as one of the possible ways to integrate with Breakpad)
Replaced CHECK with "if()"

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode62
lib/asan/asan_win.cc:62: return fwrite(buf, 1, count, stderr);
On 2012/02/08 15:30:09, glider wrote:
> The point of AsanWrite is to avoid memory allocations during writes,
that's why
> we're using syscalls on Linux and Mac.
I know, that's why I've added a FIXME.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode142
lib/asan/asan_win.cc:142: IMAGEHLP_LINE64 info;
On 2012/02/08 15:30:09, glider wrote:
> Shouldn't this be arch-dependent?
No, see the link in the code above
+
http://msdn.microsoft.com/en-us/library/ms681330(v=vs.85).aspx

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode155
lib/asan/asan_win.cc:155: " %s+0x%lx", symbol->Name, offset);
On 2012/02/08 15:30:09, glider wrote:
> Again, http://mail.python.org/pipermail/patches/2000-June/000871.html

Done.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode201
lib/asan/asan_win.cc:201: // FIXME: is __declspec enough?
On 2012/02/08 15:30:09, glider wrote:
> You may want to take a look at the TLS implementation in Chrome (I've
no idea
> :))
That's what FIXME is about.
I'd like to decide later.

http://codereview.appspot.com/5647052/diff/4001/lib/asan/asan_win.cc#newcode231
lib/asan/asan_win.cc:231: return getenv(name);
On 2012/02/08 15:30:09, glider wrote:
> Does getenv() allocate memory? If so, you may run into a trouble.
Hm, in fact we can just return NULL for now as it's only used for
ASAN_OPTIONS.

http://codereview.appspot.com/5647052/