search

eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.29k stars 723 forks source link

Potential corruption in Thread.getStackTrace() #14896

Open gacholio opened 2 years ago

gacholio commented 2 years ago

https://github.com/eclipse-openj9/openj9/blob/52f4f77546a8ab2b0f8aeac69f08b234e922498f/runtime/jcl/common/getstacktrace.c#L37-L71

The above code halts the targetThread then walks the stack of that thread caching PCs. The targetThread is then resumed, and the cache is used to create a Throwable.

The main issue here is that the stack walker may use the unused portion of the target thread stack as the cache, so when the thread is resumed, the cache may be overwritten.

To complicate matters further, resumeThreadForInspection may release and reacquire VM access, so it's not just a matter of moving the resume to after the object creation.

Several solutions present:

We're in a JNI method in this code, so we can just push/pop the object. The drawback of this solution is more time spent halted.

Could potentially overload the "cache allocated" flag to have this meaning if specified by the stack walk caller.

The stack walker can be called with a NULL current thread, but there are no such callers in the VM today which using the cache, so asserting that there's no caching in the NULL currentThread case should be sufficient.

I prefer this solution for simplicity and safety (why modify another thread when you don't have to?).

Comments? @tajila @pshipton

gacholio commented 2 years ago

In fact, rather than asserting, we could simply always malloc the cache in the NULL currentThread case.

pshipton commented 2 years ago

Sounds reasonable to me. I assume we check there is enough room on the stack to cache, and malloc if there isn't?

gacholio commented 2 years ago

Sounds reasonable to me. I assume we check there is enough room on the stack to cache, and malloc if there isn't?

Exactly.

tajila commented 2 years ago

No concerns with

Change the stack walker to cache using the current thread stack

gacholio commented 2 years ago

Turns out this isn't currently an issue due to this: https://github.com/eclipse-openj9/openj9/blob/cd9f62398f91cb7b67b487ae967a7a13117828da/runtime/vm/swalk.c#L556 If the walkState in use is not the freebie one in the walkThread, then the cache is always allocated.

gacholio commented 2 years ago

I will investigate lifting the above restriction while implementing the suggested change.