[libcxx-commits] [PATCH] D123428: [libunwind] Add configuration to disable sigreturn frame check

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai added a comment.

In D123428#3444212 <https://reviews.llvm.org/D123428#3444212>, @MaskRay wrote:

> In D123428#3444198 <https://reviews.llvm.org/D123428#3444198>, @rprichard wrote:
>
>> In D123428#3444170 <https://reviews.llvm.org/D123428#3444170>, @smeenai wrote:
>>
>>> In D123428#3444134 <https://reviews.llvm.org/D123428#3444134>, @rprichard wrote:
>>>
>>>> Using /proc/self/maps would be subject to TOCTOU, but I think most methods wouldn't, e.g.:
>>>>
>>>> - Open /proc/self/mem and pread() the address. This seems strictly better than /proc/self/maps?
>>>> - Create a pipe using pipe(), write() the bytes into the pipe buffer and read() them back out. I believe a Linux pipe buffer is guaranteed to be big enough (>= 8 bytes).
>>>> - process_vm_readv
>>>>
>>>> I wonder if security configurations are a problem. Maybe I should experiment on an Android build.
>>>
>>> With all those methods, there's still the chance (however unlikely) that the address is readable at the time of the check but somehow becomes unreadable by the time we perform the actual read, right? I doubt it matters much in practice though.
>>
>> The unwinder needs to read two aligned 4-byte words at the target PC. I was thinking that it could replace the ordinary reads with system calls. e.g. For the pipe trick, write the target 8 bytes into a pipe and then read them back out. If multiple threads share a pipe fd, then they'd need the synchronize their use of the pipe.
>>
>> I wonder if we'd need to close the pipe fd for dlclose. On Android, the unwinder is linked statically into app shared objects.
>>
>>> For the pipe, since we'll be checking for validity multiple times, I imagine we'll need to empty out the buffer at some point. libunwind seems to use `mincore` if available and the pipe as a fallback, but I'm not really understanding how `mincore` would work here, since a page could be readable even though it's not currently resident.
>>
>> Yeah, `mincore` doesn't seem to do what's needed here.
>
> https://github.com/abseil/abseil-cpp/blob/master/absl/debugging/internal/address_is_readable.cc has gone through several iterations. We can use `rt_sigprocmask`.

@rprichard, do you know if this would work for Android? It has the TOCTOU issue, but I imagine it's much simpler than having to manage and synchronize the pipe fd, and we could live with the TOCTOU in practice.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123428/new/

https://reviews.llvm.org/D123428