Use the intrinsic version of the Interlocked* functions.

Use the intrinsic version of the Interlocked* functions.

This save us a system call and allows this code to be instrumented by the memory testing tools.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276701

[Sébastien Marchand, 2014-06-02 19:05:31]

PTAL.

Do you think that I should also use the intrinsic version of
InterlockedExchange* (like we do here:
https://code.google.com/p/chromium/codesearch#chromium/src/sandbox/win/src/sa...)
?

[Sigurður Ásgeirsson, 2014-06-02 19:43:08]

nice - do you need to do this for the ia32 and the x64 #ifdef branches
separately, perhaps?

https://codereview.chromium.org/308683011/diff/20001/base/atomicops_internals...
File base/atomicops_internals_x86_msvc.h (right):

https://codereview.chromium.org/308683011/diff/20001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:25: #pragma
intrinsic(_InterlockedCompareExchange)
I'm concerned that you're setting this for all clients of this file. Maybe
that's benign, but I just don't know. Another thing that bugs me is that
winbase.h seems to define these functions to their intrinsic counterparts. Why
isn't that hitting for these invocations?

[Sébastien Marchand, 2014-06-03 14:39:48]

I've put this between ifdefs, it seems to work fine... I'll do more tests later.

https://codereview.chromium.org/308683011/diff/20001/base/atomicops_internals...
File base/atomicops_internals_x86_msvc.h (right):

https://codereview.chromium.org/308683011/diff/20001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:25: #pragma
intrinsic(_InterlockedCompareExchange)
On 2014/06/02 19:43:08, Sigurður Ásgeirsson wrote:
> I'm concerned that you're setting this for all clients of this file. Maybe
> that's benign, but I just don't know. Another thing that bugs me is that
> winbase.h seems to define these functions to their intrinsic counterparts. Why
> isn't that hitting for these invocations?

winbase.h seems to only define these functions to their intrinsic counterpart in
some cases:
- if defined(_M_IA64) && !defined(RC_INVOKED)
- if defined(_M_ARM) && !defined(RC_INVOKED)
- if defined(_M_AMD64) && !defined(RC_INVOKED)

[Sigurður Ásgeirsson, 2014-06-03 15:03:08]

nice!

https://codereview.chromium.org/308683011/diff/40001/base/atomicops_internals...
File base/atomicops_internals_x86_msvc.h (right):

https://codereview.chromium.org/308683011/diff/40001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:29: #pragma
intrinsic(_InterlockedCompareExchange)
Stupid question, but why do you need to declare the intrinsics like this. Won't
<intrin.h> do that for you?

https://codereview.chromium.org/308683011/diff/40001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:53: LONG result =
_InterlockedCompareExchange(
maybe a one-line comment that Interlocked* doesn't resolve to intrinsics on x86?

[Sébastien Marchand, 2014-06-03 19:58:46]

After second thought I've removed the ifdefs x86|x64... I don't think that this
is necessary (I'll confirm it locally, but the intrinsic functions seems to be
available in the x64 compiler).

https://codereview.chromium.org/308683011/diff/40001/base/atomicops_internals...
File base/atomicops_internals_x86_msvc.h (right):

https://codereview.chromium.org/308683011/diff/40001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:29: #pragma
intrinsic(_InterlockedCompareExchange)
On 2014/06/03 15:03:08, Sigurður Ásgeirsson wrote:
> Stupid question, but why do you need to declare the intrinsics like this.
Won't
> <intrin.h> do that for you?

I don't need to, I've looked at 'sandbox/win/src/sandbox_nt_util.h' as an
example but I think that the code in this file could be cleaned too...

https://codereview.chromium.org/308683011/diff/40001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:53: LONG result =
_InterlockedCompareExchange(
On 2014/06/03 15:03:08, Sigurður Ásgeirsson wrote:
> maybe a one-line comment that Interlocked* doesn't resolve to intrinsics on
x86?

Done.

[Sigurður Ásgeirsson, 2014-06-04 14:48:35]

missing upload?

[Sébastien Marchand, 2014-06-04 14:49:44]

Yeah sorry, I've switched to another branch before uploading... I'll ping you
once I've uploaded a new patchset.

[Sébastien Marchand, 2014-06-04 15:07:21]

And here it is.

[Sigurður Ásgeirsson, 2014-06-04 15:12:54]

nice, still more nits :)

https://codereview.chromium.org/308683011/diff/60001/base/atomicops_internals...
File base/atomicops_internals_x86_msvc.h (right):

https://codereview.chromium.org/308683011/diff/60001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:30: #pragma
intrinsic(_InterlockedCompareExchange)
do you need even this? Doesn't <intrin.h> declare this for you?

[Sébastien Marchand, 2014-06-04 15:21:02]

https://codereview.chromium.org/308683011/diff/60001/base/atomicops_internals...
File base/atomicops_internals_x86_msvc.h (right):

https://codereview.chromium.org/308683011/diff/60001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:30: #pragma
intrinsic(_InterlockedCompareExchange)
On 2014/06/04 15:12:54, Sigurður Ásgeirsson wrote:
> do you need even this? Doesn't <intrin.h> declare this for you?

Nop, it seems that I've to explicitly use the intrinsic pragma...
(http://msdn.microsoft.com/en-us/library/26td21ds(v=vs.100).aspx, and see here
for an example http://msdn.microsoft.com/en-us/library/f24ya7ct(v=vs.100).aspx)

I wonder if we could use the /Oi compiler flag on our builds....
(http://msdn.microsoft.com/en-us/library/f99tchzc(v=vs.100).aspx)

[Sébastien Marchand, 2014-06-04 15:25:03]

nvm, we already enable it for the official builds
(https://code.google.com/p/chromium/codesearch#chromium/src/build/internal/rel...)

[Sigurður Ásgeirsson, 2014-06-04 15:29:25]

Nice - is this also true for ASAN builds?


On Wed, Jun 4, 2014 at 11:25 AM, <sebmarchand@chromium.org> wrote:

> nvm, we already enable it for the official builds
> (https://code.google.com/p/chromium/codesearch#chromium/
> src/build/internal/release_impl_official.gypi&q=
> EnableIntrinsicFunctions&sq=package:chromium&type=cs&l=10)
>
> https://codereview.chromium.org/308683011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sébastien Marchand, 2014-06-04 15:32:35]

Yeah, but he /Oi flag seems to only apply to the CRT functions , we still need
to do this for the Interlocked* functions (and maybe some other ones...)

[Sigurður Ásgeirsson, 2014-06-04 15:33:42]

I see no difference in the generated test code, whether I declare the
intrinsic or not.



On Wed, Jun 4, 2014 at 11:32 AM, <sebmarchand@chromium.org> wrote:

> Yeah, but he /Oi flag seems to only apply to the CRT functions , we still
> need
> to do this for the Interlocked* functions (and maybe some other ones...)
>
> https://codereview.chromium.org/308683011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sébastien Marchand, 2014-06-04 15:37:49]

Hum, Here's my test case:



class Foo : public base::RefCountedThreadSafe<Foo> {
  int test;
};

TEST_F(HeapTest, InterlockedFunctions) {
  scoped_refptr<Foo> test = new Foo();
  test->Release();
  test->Release();
}



Without my patch I get a CORRUPT_BLOCK error when we flush the quarantine,
with it I catch the double free attempt...


2014-06-04 11:33 GMT-04:00 Sigurður Ásgeirsson <siggi@chromium.org>:

> I see no difference in the generated test code, whether I declare the
> intrinsic or not.
>
>
>
> On Wed, Jun 4, 2014 at 11:32 AM, <sebmarchand@chromium.org> wrote:
>
>> Yeah, but he /Oi flag seems to only apply to the CRT functions , we still
>> need
>> to do this for the Interlocked* functions (and maybe some other ones...)
>>
>> https://codereview.chromium.org/308683011/
>>
>
>


-- 

Sébastien Marchand | Software Developer |  sebmarchand@google.com

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sigurður Ásgeirsson, 2014-06-04 15:47:27]

Is this both in debug and release?


On Wed, Jun 4, 2014 at 11:37 AM, Sébastien Marchand <
sebmarchand@chromium.org> wrote:

> Hum, Here's my test case:
>
>
>
> class Foo : public base::RefCountedThreadSafe<Foo> {
>   int test;
> };
>
> TEST_F(HeapTest, InterlockedFunctions) {
>   scoped_refptr<Foo> test = new Foo();
>   test->Release();
>   test->Release();
> }
>
>
>
> Without my patch I get a CORRUPT_BLOCK error when we flush the quarantine,
> with it I catch the double free attempt...
>
>
> 2014-06-04 11:33 GMT-04:00 Sigurður Ásgeirsson <siggi@chromium.org>:
>
> I see no difference in the generated test code, whether I declare the
>> intrinsic or not.
>>
>>
>>
>> On Wed, Jun 4, 2014 at 11:32 AM, <sebmarchand@chromium.org> wrote:
>>
>>> Yeah, but he /Oi flag seems to only apply to the CRT functions , we
>>> still need
>>> to do this for the Interlocked* functions (and maybe some other ones...)
>>>
>>> https://codereview.chromium.org/308683011/
>>>
>>
>>
>
>
> --
>
> Sébastien Marchand | Software Developer |  sebmarchand@google.com
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sébastien Marchand, 2014-06-04 15:50:03]

Release only, Here's my test case (I'm playing with the /Oi flag to see if
it change something):

# Compile without my changes to atomicops_internals_x86_msvc.h

[parser]E:\src\syzygy\src\build\Release>instrument.exe --mode=asan
--input-image=syzyasan_rtl_unittests.exe
--output-image=syzyasan_rtl_unittests_asan.exe --debug-friendly --overwrite
[parser]E:\src\syzygy\src\build\Release>agent_logger start --
syzyasan_rtl_unittests_asan.exe --gtest_filter=HeapTest.InterlockedFunctions
[0604/114006:INFO:application_impl.h(45)] Syzygy AgentLogger Version
0.7.16.0 (4db8767).
[0604/114006:INFO:application_impl.h(47)] Copyright (c) Google Inc. All
rights reserved.
[0604/114006:INFO:agent_logger_app.cc(288)] Using logger instance ID: ''.
[0604/114006:INFO:agent_logger_app.cc(289)] Writing minidumps to:
E:\src\syzygy\src\build\Release
[0604/114006:INFO:service.cc(62)] Starting the Logger service with instance
ID "".
[0604/114006:INFO:agent_logger.cc(145)] Starting the logging service.
[0604/114006:INFO:agent_logger_app.cc(136)] Launching
'syzyasan_rtl_unittests_asan.exe'.
PID=1088; cmd-line='syzyasan_rtl_unittests_asan.exe
--gtest_filter=HeapTest.InterlockedFunctions'
SyzyASAN: Using default error reporting handler.
Note: Google Test filter = HeapTest.InterlockedFunctions
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HeapTest
[ RUN      ] HeapTest.InterlockedFunctions
SyzyASAN error: heap-use-after-free on address 0x00547513
(stack_id=0x00540D89)
WRITE of size 4 at 0x00547513
    #0 0x000000f002eb in base::subtle::RefCountedThreadSafeBase::Release
e:\src\syzygy\src\base\memory\ref_counted.cc:84
    #1 0x000000ea2258 in
agent::asan::HeapTest_InterlockedFunctions_Test::TestBody
e:\src\syzygy\src\syzygy\agent\asan\asan_heap_unittest.cc:1769
    #2 0x000000f1d0dd in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,void>
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2063
    #3 0x000000f2967a in testing::Test::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2151
    #4 0x000000f29c46 in testing::TestInfo::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2330
    #5 0x000000f2994d in testing::TestCase::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2443
    #6 0x000000f2a4e9 in testing::internal::UnitTestImpl::RunAllTests
e:\src\syzygy\src\testing\gtest\src\gtest.cc:4314
    #7 0x000000f1d266 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
e:\src\syzygy\src\testing\gtest\src\gte
st.cc:2063
    #8 0x000000f29e9c in testing::UnitTest::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:3929
    #9 0x000000ec7168 in main
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_unittests_main.cc:23
    #10 0x000000f3eb33 in __tmainCRTStartup
f:\dd\vctools\crt\crtw32\startup\crt0.c:255
    #11 0x000076ba339c in BaseThreadInitThunk
    #12 0x000077279fd5 in RtlInitializeExceptionChain
    #13 0x000077279f7b in RtlInitializeExceptionChain
00547513 is 3 bytes inside 8-byte block [00547510,00547518)
freed here:
    #0 0x000010fbe0b6 in agent::asan::HeapProxy::Free
e:\src\syzygy\src\syzygy\agent\asan\asan_heap.cc:419
    #1 0x000010fb6e7e in asan_HeapFree
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_impl.cc:534
    #2 0x000000f3d839 in free f:\dd\vctools\crt\crtw32\heap\free.c:51
    #3 0x000000ea2244 in
agent::asan::HeapTest_InterlockedFunctions_Test::TestBody
e:\src\syzygy\src\syzygy\agent\asan\asan_heap_unittest.cc:1768
    #4 0x000000f2967a in testing::Test::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2151
    #5 0x000000f2994d in testing::TestCase::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2443
    #6 0x000000f2a4e9 in testing::internal::UnitTestImpl::RunAllTests
e:\src\syzygy\src\testing\gtest\src\gtest.cc:4314
    #7 0x000000f1d266 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
e:\src\syzygy\src\testing\gtest\src\gte
st.cc:2063
    #8 0x000000f29e9c in testing::UnitTest::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:3929
    #9 0x000000ec7168 in main
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_unittests_main.cc:23
    #10 0x000000f3eb33 in __tmainCRTStartup
f:\dd\vctools\crt\crtw32\startup\crt0.c:255
    #11 0x000076ba339c in BaseThreadInitThunk
    #12 0x000077279fd5 in RtlInitializeExceptionChain
    #13 0x000077279f7b in RtlInitializeExceptionChain
previously allocated here:
    #0 0x000010fbcd52 in agent::asan::HeapProxy::Alloc
e:\src\syzygy\src\syzygy\agent\asan\asan_heap.cc:269
    #1 0x000010fb6b6e in asan_HeapAlloc
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_impl.cc:506
    #2 0x000000f43f5d in malloc f:\dd\vctools\crt\crtw32\heap\malloc.c:92
    #3 0x000000f3c726 in operator new
f:\dd\vctools\crt\crtw32\heap\new.cpp:59
    #4 0x000000ea21d6 in
agent::asan::HeapTest_InterlockedFunctions_Test::TestBody
e:\src\syzygy\src\syzygy\agent\asan\asan_heap_unittest.cc:1767
    #5 0x000000f2967a in testing::Test::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2151
    #6 0x000000f2994d in testing::TestCase::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2443
    #7 0x000000f2a4e9 in testing::internal::UnitTestImpl::RunAllTests
e:\src\syzygy\src\testing\gtest\src\gtest.cc:4314
    #8 0x000000f1d266 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
e:\src\syzygy\src\testing\gtest\src\gte
st.cc:2063
    #9 0x000000f29e9c in testing::UnitTest::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:3929
    #10 0x000000ec7168 in main
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_unittests_main.cc:23
    #11 0x000000f3eb33 in __tmainCRTStartup
f:\dd\vctools\crt\crtw32\startup\crt0.c:255
    #12 0x000076ba339c in BaseThreadInitThunk
    #13 0x000077279fd5 in RtlInitializeExceptionChain
    #14 0x000077279f7b in RtlInitializeExceptionChain
Shadow bytes around the buggy address:
  0x00547400: 00 00 00 00 00 00 00 00
  0x00547440: 00 00 00 00 00 00 00 00
  0x00547480: 00 00 00 00 00 00 00 00
  0x005474c0: 00 00 00 00 00 00 00 00
=>0x00547500: f4 f4[fd]fb fb fb 00 00
  0x00547540: 00 00 00 00 00 00 00 00
  0x00547580: 00 00 00 00 00 00 00 00
  0x005475c0: 00 00 00 00 00 00 00 00
  0x00547600: 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  ASan memory byte:     f1
  Invalid address:     f2
  User redzone:     f3
  Block header redzone:     f4
  Heap left redzone:     fa
  Heap righ redzone:     fb
  ASan reserved byte:     fc
  Freed Heap region:     fd
[0604/114009:INFO:service.cc(82)] Stopping the Logger service with instance
ID "".
[0604/114009:INFO:service.cc(101)] Joining the Logger service with instance
ID "".
[0604/114009:INFO:agent_logger.cc(549)] The logging service has stopped.



# Compile with my changes to atomicops_internals_x86_msvc.h
[parser]E:\src\syzygy\src\build\Release>instrument.exe --mode=asan
--input-image=syzyasan_rtl_unittests.exe
--output-image=syzyasan_rtl_unittests_asan.exe --debug-friendly --overwrite
[parser]E:\src\syzygy\src\build\Release>agent_logger start --
syzyasan_rtl_unittests_asan.exe --gtest_filter=HeapTest.InterlockedFunctions
[0604/114408:INFO:application_impl.h(45)] Syzygy AgentLogger Version
0.7.16.0 (4db8767).
[0604/114408:INFO:application_impl.h(47)] Copyright (c) Google Inc. All
rights reserved.
[0604/114408:INFO:agent_logger_app.cc(288)] Using logger instance ID: ''.
[0604/114408:INFO:agent_logger_app.cc(289)] Writing minidumps to:
E:\src\syzygy\src\build\Release
[0604/114408:INFO:service.cc(62)] Starting the Logger service with instance
ID "".
[0604/114408:INFO:agent_logger.cc(145)] Starting the logging service.
[0604/114408:INFO:agent_logger_app.cc(136)] Launching
'syzyasan_rtl_unittests_asan.exe'.
PID=19964; cmd-line='syzyasan_rtl_unittests_asan.exe
--gtest_filter=HeapTest.InterlockedFunctions'
SyzyASAN: Using default error reporting handler.
Note: Google Test filter = HeapTest.InterlockedFunctions
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from HeapTest
[ RUN      ] HeapTest.InterlockedFunctions
[       OK ] HeapTest.InterlockedFunctions (191 ms)
[----------] 1 test from HeapTest (193 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (198 ms total)
[  PASSED  ] 1 test.
SyzyASAN error: corrupted-block on address 0x00697500 (stack_id=0x004AF2C6)
    #0 0x000010b4047c in agent::asan::HeapProxy::TrimQuarantine
e:\src\syzygy\src\syzygy\agent\asan\asan_heap.cc:614
    #1 0x000010b3d6b4 in agent::asan::HeapProxy::Destroy
e:\src\syzygy\src\syzygy\agent\asan\asan_heap.cc:239
    #2 0x000010b368f8 in agent::asan::TearDownRtl
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_impl.cc:65
    #3 0x000010b36272 in DllMain
e:\src\syzygy\src\syzygy\agent\asan\syzyasan_rtl.cc:272
    #4 0x000010b56b20 in __DllMainCRTStartup
f:\dd\vctools\crt\crtw32\startup\dllcrt0.c:377
    #5 0x000010b56a51 in _DllMainCRTStartup
f:\dd\vctools\crt\crtw32\startup\dllcrt0.c:340
    #6 0x000077279be1 in RtlQueryEnvironmentVariable
    #7 0x00007728d843 in LdrShutdownProcess
    #8 0x00007728d618 in RtlExitUserProcess
    #9 0x000076ba7a02 in ExitProcess
    #10 0x000000abf049 in __crtExitProcess
f:\dd\vctools\crt\crtw32\startup\crt0dat.c:774
    #11 0x000000abf3ef in doexit
f:\dd\vctools\crt\crtw32\startup\crt0dat.c:678
    #12 0x000000abf30a in exit
f:\dd\vctools\crt\crtw32\startup\crt0dat.c:417
    #13 0x000000abeb5c in __tmainCRTStartup
f:\dd\vctools\crt\crtw32\startup\crt0.c:264
    #14 0x000076ba339c in BaseThreadInitThunk
    #15 0x000077279fd5 in RtlInitializeExceptionChain
    #16 0x000077279f7b in RtlInitializeExceptionChain
freed here:
    #0 0x000010b3dfd6 in agent::asan::HeapProxy::Free
e:\src\syzygy\src\syzygy\agent\asan\asan_heap.cc:419
    #1 0x000010b36e7e in asan_HeapFree
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_impl.cc:534
    #2 0x000000abd8a0 in free f:\dd\vctools\crt\crtw32\heap\free.c:51
    #3 0x000000a222fa in
agent::asan::HeapTest_InterlockedFunctions_Test::TestBody
e:\src\syzygy\src\syzygy\agent\asan\asan_heap_unittest.cc:1768
    #4 0x000000aa96ba in testing::Test::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2151
    #5 0x000000aa998d in testing::TestCase::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2443
    #6 0x000000aaa529 in testing::internal::UnitTestImpl::RunAllTests
e:\src\syzygy\src\testing\gtest\src\gtest.cc:4314
    #7 0x000000a9d2d9 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
e:\src\syzygy\src\testing\gtest\src\gte
st.cc:2063
    #8 0x000000aa9edc in testing::UnitTest::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:3929
    #9 0x000000a47038 in main
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_unittests_main.cc:23
    #10 0x000000abeb38 in __tmainCRTStartup
f:\dd\vctools\crt\crtw32\startup\crt0.c:255
    #11 0x000076ba339c in BaseThreadInitThunk
    #12 0x000077279fd5 in RtlInitializeExceptionChain
    #13 0x000077279f7b in RtlInitializeExceptionChain
previously allocated here:
    #0 0x000010b3cc72 in agent::asan::HeapProxy::Alloc
e:\src\syzygy\src\syzygy\agent\asan\asan_heap.cc:269
    #1 0x000010b36b6e in asan_HeapAlloc
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_impl.cc:506
    #2 0x000000ac3f62 in malloc f:\dd\vctools\crt\crtw32\heap\malloc.c:92
    #3 0x000000abc72b in operator new
f:\dd\vctools\crt\crtw32\heap\new.cpp:59
    #4 0x000000a2228c in
agent::asan::HeapTest_InterlockedFunctions_Test::TestBody
e:\src\syzygy\src\syzygy\agent\asan\asan_heap_unittest.cc:1767
    #5 0x000000aa96ba in testing::Test::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2151
    #6 0x000000aa998d in testing::TestCase::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:2443
    #7 0x000000aaa529 in testing::internal::UnitTestImpl::RunAllTests
e:\src\syzygy\src\testing\gtest\src\gtest.cc:4314
    #8 0x000000a9d2d9 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool>
e:\src\syzygy\src\testing\gtest\src\gte
st.cc:2063
    #9 0x000000aa9edc in testing::UnitTest::Run
e:\src\syzygy\src\testing\gtest\src\gtest.cc:3929
    #10 0x000000a47038 in main
e:\src\syzygy\src\syzygy\agent\asan\asan_rtl_unittests_main.cc:23
    #11 0x000000abeb38 in __tmainCRTStartup
f:\dd\vctools\crt\crtw32\startup\crt0.c:255
    #12 0x000076ba339c in BaseThreadInitThunk
    #13 0x000077279fd5 in RtlInitializeExceptionChain
    #14 0x000077279f7b in RtlInitializeExceptionChain
[0604/114410:INFO:service.cc(82)] Stopping the Logger service with instance
ID "".
[0604/114410:INFO:service.cc(101)] Joining the Logger service with instance
ID "".
[0604/114410:INFO:agent_logger.cc(549)] The logging service has stopped.



2014-06-04 11:47 GMT-04:00 Sigurður Ásgeirsson <siggi@chromium.org>:

> Is this both in debug and release?
>
>
> On Wed, Jun 4, 2014 at 11:37 AM, Sébastien Marchand <
> sebmarchand@chromium.org> wrote:
>
>> Hum, Here's my test case:
>>
>>
>>
>> class Foo : public base::RefCountedThreadSafe<Foo> {
>>   int test;
>> };
>>
>> TEST_F(HeapTest, InterlockedFunctions) {
>>   scoped_refptr<Foo> test = new Foo();
>>   test->Release();
>>   test->Release();
>> }
>>
>>
>>
>> Without my patch I get a CORRUPT_BLOCK error when we flush the
>> quarantine, with it I catch the double free attempt...
>>
>>
>> 2014-06-04 11:33 GMT-04:00 Sigurður Ásgeirsson <siggi@chromium.org>:
>>
>> I see no difference in the generated test code, whether I declare the
>>> intrinsic or not.
>>>
>>>
>>>
>>> On Wed, Jun 4, 2014 at 11:32 AM, <sebmarchand@chromium.org> wrote:
>>>
>>>> Yeah, but he /Oi flag seems to only apply to the CRT functions , we
>>>> still need
>>>> to do this for the Interlocked* functions (and maybe some other ones...)
>>>>
>>>> https://codereview.chromium.org/308683011/
>>>>
>>>
>>>
>>
>>
>> --
>>
>>  Sébastien Marchand | Software Developer |  sebmarchand@google.com
>>
>>
>


-- 

Sébastien Marchand | Software Developer |  sebmarchand@google.com

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sébastien Marchand, 2014-06-05 17:27:38]

I've tried this patch on an instrumented Official build and I'm having the same
result, with this one I'm catching the use-after-free when I tried to release a
RefCountedThreadSafe pointer twice, without it no bug triggered (we only get
some info about the corruption when we check the heap).

[Sigurður Ásgeirsson, 2014-06-05 17:41:09]

lgtm

https://codereview.chromium.org/308683011/diff/60001/base/atomicops_internals...
File base/atomicops_internals_x86_msvc.h (right):

https://codereview.chromium.org/308683011/diff/60001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:28: // The Interlocked* functions doesn't
resolve to their intrinsics
nit: doesn't ->don't

Also mention this is on ia32/x86 only, as from what I see on x64 the
Interlocked* functions default to defines directing them to intrinsics?

https://codereview.chromium.org/308683011/diff/60001/base/atomicops_internals...
base/atomicops_internals_x86_msvc.h:30: #pragma
intrinsic(_InterlockedCompareExchange)
On 2014/06/04 15:21:02, Sébastien Marchand wrote:
> On 2014/06/04 15:12:54, Sigurður Ásgeirsson wrote:
> > do you need even this? Doesn't <intrin.h> declare this for you?
> 
> Nop, it seems that I've to explicitly use the intrinsic pragma...
> (http://msdn.microsoft.com/en-us/library/26td21ds(v=vs.100).aspx, and see here
> for an example
http://msdn.microsoft.com/en-us/library/f24ya7ct(v=vs.100).aspx)
> 
> I wonder if we could use the /Oi compiler flag on our builds....
> (http://msdn.microsoft.com/en-us/library/f99tchzc(v=vs.100).aspx)

Ah, interesting. Turns out <intrin.h> gives you declarations for these functions
but no #pragma intrin ...

[Sébastien Marchand, 2014-06-10 20:27:36]

It looks like that I don't even have to declare the intrinsics on x86...
Including intrin.h and using the _Interlocked* version of the functions (note
the underscore) seems to be enough ! Confirmed in x86 and x64 by looking at the
generated code.

[Sébastien Marchand, 2014-06-11 13:13:30]

+ajwong@ for owner approval.

[awong, 2014-06-11 17:04:14]

LGTM

[Sébastien Marchand, 2014-06-11 17:26:17]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-06-11 17:29:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/308683011/80001

[commit-bot: I haz the power, 2014-06-11 21:04:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:36:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:36:54]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Sébastien Marchand, 2014-06-12 13:44:35]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2014-06-12 13:46:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/308683011/80001

[commit-bot: I haz the power, 2014-06-12 16:17:31]

Change committed as 276701