Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upFix asan false-positive errors #15563
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test |
This comment has been minimized.
This comment has been minimized.
|
@janvorli PTAL |
janvorli
reviewed
Dec 19, 2017
|
|
||
| // Asan also uses alternate stack for signal handling so we cannot free it | ||
| // in this case otherwise we will get bad-free error. | ||
| #ifndef HAS_ASAN | ||
| free(oss.ss_sp); |
This comment has been minimized.
This comment has been minimized.
janvorli
Dec 19, 2017
Member
So this means that we will leak the memory we have allocated for the alternate stack when ASAN is enabled. That would happen for every thread that terminates. I also wonder if ASAN allocates large enough alternate stack. And what does it do with the alternate stack we have allocated? Does it just overwrite it with its own?
This comment has been minimized.
This comment has been minimized.
kbaladurin
Dec 19, 2017
Author
Collaborator
Asan sets alternate stack before coreclr tries to do it. So when EnsureSignalAlternateStack is called it checks that alternate stack already exists and returns:
BOOL EnsureSignalAlternateStack()
{
stack_t oss;
// Query the current alternate signal stack
int st = sigaltstack(NULL, &oss);
if ((st == 0) && (oss.ss_flags == SS_DISABLE))
{
...
}
return (st == 0);
}As EnsureSignalAlternateStack doesn't allocate memory FreeSignalAlternateStack shouldn't free it.
Size of Asan's alternate stack is 4 * SIGSTKSZ (sanitizer_posix_libcdep.cc), coreclr sets alternate with the following size:
int altStackSize = SIGSTKSZ + ALIGN_UP(sizeof(SignalHandlerWorkerReturnPoint), 16) + GetVirtualPageSize();Using option use_sigaltstack we can disable setting alternate stack in Asan, but as coreclr's size of the alternate stack could be less than Asan's one it can lead to some problems. For example Android also sets alternate stack but it's too small for Asan:
// If the alternate stack is already in place, do nothing.
// Android always sets an alternate stack, but it's too small for us.
if (!SANITIZER_ANDROID && !(oldstack.ss_flags & SS_DISABLE)) return;
This comment has been minimized.
This comment has been minimized.
janvorli
Jan 18, 2018
Member
@kbaladurin I am sorry for a late response, I was on a paternity leave for the last month. Looking at the code in the sanitizer, their alt stack allocation doesn't create a guard page that enables catching stack overflow. So I'd prefer disabling their creation of the alt stack using the option you have mentioned and rather increase our alt stack size if the HAS_ASAN is defined to the value they use (resp, to the max of what we use and they use, to stay on the safe size).
This comment has been minimized.
This comment has been minimized.
kbaladurin
force-pushed the
kbaladurin:asan
branch
from
854dfa4
to
0957ced
Jan 19, 2018
janvorli
approved these changes
Jan 19, 2018
|
LGTM, thank you! |
kbaladurin
force-pushed the
kbaladurin:asan
branch
from
0957ced
to
dd11563
Jan 22, 2018
This comment has been minimized.
This comment has been minimized.
|
@janvorli thank you! I've updated comments in |
This comment has been minimized.
This comment has been minimized.
|
@dotnet-bot test Ubuntu arm Cross Debug Build |
This comment has been minimized.
This comment has been minimized.
|
@dotnet-bot test Ubuntu arm Cross Debug Innerloop Build |
This comment has been minimized.
This comment has been minimized.
|
@dotnet-bot help |
This comment has been minimized.
This comment has been minimized.
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
This comment has been minimized.
This comment has been minimized.
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
This comment has been minimized.
This comment has been minimized.
|
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
kbaladurin
force-pushed the
kbaladurin:asan
branch
from
dd11563
to
2ce254d
Jan 25, 2018
This comment has been minimized.
This comment has been minimized.
|
@dotnet-bot test CentOS7.1 x64 Release Innerloop Build and Test |
This comment has been minimized.
This comment has been minimized.
|
The CentOS7.1 x64 is failing everywhere today. |


kbaladurin commentedDec 18, 2017
•
edited
Fix asan false-positive errors:
Fixes false-positive buffer-overflow (buffer-underflow) errors during stack unwinding (#15540)