RFR: 8297138: UB leading to crash in Amalloc with optimized builds

[Thomas Stuefe]

On Tue, 29 Nov 2022 15:04:29 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>>> I think we can close this PR in favour of a RFE that simply removes `UseMallocOnly`. It may have been useful whilst VM memory management was in its infancy (it was introduced in 1998) but now it is just a liability. I think we can simply remove it (and not worry if Arenas need better debuggability or error tracking - that would be a separate RFE).
>> 
>> I'm very much for it!
>> 
>> But we may need a way to detect memory stomping across arena allocations. See https://bugs.openjdk.org/browse/JDK-8007475, and hotspot/jtreg/runtime/8007475/StackMapFrameTest.java. I have used UseMallocOnly for similar purposes in the past.
>> 
>> As I wrote before when I did some Arena code massaging last year (8263557, 8263558, etc, see git log) I also started to add per-arena-allocation-canaries. I hit some minor road blocked and then stopped because I was not sure it was even wanted. 
>> 
>> Having canaries in Arenas would also have the advantage of being both faster and leaner than what we do now with C-heap allocations. So we could afford to enable them for more tests.
>> 
>> So if you all agree, I could bookmark some time early in the jdk20 time frame to revive my patch. We could remove UseMallocOnly now, or do it together with the proposed canary change.
>
>> @tstuefe is your Arena canary patch as much code as UseMallocOnly? I'm writing the RFE now and the argument for removing UseMallocOnly is to not have this extra code to maintain that nobody really uses. I guess we can see once you send a PR but hopefully you can share code with os::malloc or something. Thanks for all your comments on this PR.
> 
> Hi @coleenp, you may not like it: https://github.com/openjdk/jdk/compare/master...tstuefe:jdk:arena-remove-usemalloc-and-guard-allocs . Note that I mixed a bunch of cleanups and comments in there, if I were to do it again I would do it in a more lightweight fashion.

> > @tstuefe is your Arena canary patch as much code as UseMallocOnly? I'm writing the RFE now and the argument for removing UseMallocOnly is to not have this extra code to maintain that nobody really uses. I guess we can see once you send a PR but hopefully you can share code with os::malloc or something. Thanks for all your comments on this PR.
> 
> Hi @coleenp, you may not like it: [master...tstuefe:jdk:arena-remove-usemalloc-and-guard-allocs](https://github.com/openjdk/jdk/compare/master...tstuefe:jdk:arena-remove-usemalloc-and-guard-allocs) . Note that I mixed a bunch of cleanups and comments in there, if I were to do it again I would do it in a more lightweight fashion.

Actually, a very simple way to implement arena-based canaries would be to do like Metaspace arenas and inject a pointer chain as "normal" allocation. Like this:


class Arena {
  struct canary_t { canary_t* next; .... };
  canary_t* _first_canary;
  canary_t* _last_canary;

  void* Amalloc(size s) {
    void* p_user_data = inner_Amalloc();
    canary_t* c = inner_Amalloc(sizeof(canary_t));
    // ... chain c together with _last_canary / _first_canary
    return p_user_data;
  }


then, to check for overwriters (in ~Arena or whenever you like), walk the chain. 

This piggy-backs nicely on the normal allocation paths, since the canary is allocated like any other block would be.

-------------

PR: https://git.openjdk.org/jdk/pull/11320