Undefined behaviour in hotspot

[Volker Simonis]

Hi Omair,

I've downloaded and compiled a brand new version of gcc 4.9.0 (it was
actually released today!)

I could reproduce your problem on x86. On x86_64 there's a problem as
well - the VM hangs during the initialization. Funny enough I had no
problems on ppc64. A freshly compiled VM from jdk9/hs-comp ran without
any problems. So this seems to be x86 related.

I've further analyzed the problem you described and the good news is
that it is not related to calling encoding() on a zero pointer - so
hopefully no need to massively change HotSpot code. The assembly code
of "Assembler::push(RegisterImpl* src)" looks as follows:

0xf6959174 <_ZN9Assembler4pushEP12RegisterImpl+20>:    mov    0xc(%ebp),%esi
0xf6959177 <_ZN9Assembler4pushEP12RegisterImpl+23>:    mov    0x8(%ebp),%edi
0xf695917a <_ZN9Assembler4pushEP12RegisterImpl+26>:    cmp    $0x7,%esi
0xf695917d <_ZN9Assembler4pushEP12RegisterImpl+29>:    jbe
0xf69591a3 <_ZN9Assembler4pushEP12RegisterImpl+67>

0xf695917f <_ZN9Assembler4pushEP12RegisterImpl+31>:    lea
-0x3a0c4e(%ebx),%eax
0xf6959185 <_ZN9Assembler4pushEP12RegisterImpl+37>:    push   %eax
0xf6959186 <_ZN9Assembler4pushEP12RegisterImpl+38>:    lea
-0x3a0c3d(%ebx),%eax
0xf695918c <_ZN9Assembler4pushEP12RegisterImpl+44>:    push   %eax
0xf695918d <_ZN9Assembler4pushEP12RegisterImpl+45>:    lea
-0x39be48(%ebx),%eax
0xf6959193 <_ZN9Assembler4pushEP12RegisterImpl+51>:    push   $0x41
0xf6959195 <_ZN9Assembler4pushEP12RegisterImpl+53>:    push   %eax
0xf6959196 <_ZN9Assembler4pushEP12RegisterImpl+54>:    call
0xf6c9b730 <_Z15report_vm_errorPKciS0_S0_>
0xf695919b <_ZN9Assembler4pushEP12RegisterImpl+59>:    call
0xf72bc1c0 <breakpoint>
0xf69591a0 <_ZN9Assembler4pushEP12RegisterImpl+64>:    add    $0x10,%esp

0xf69591a3 <_ZN9Assembler4pushEP12RegisterImpl+67>:    mov    0xc(%edi),%edx
0xf69591a6 <_ZN9Assembler4pushEP12RegisterImpl+70>:    mov    %esi,%eax
0xf69591a8 <_ZN9Assembler4pushEP12RegisterImpl+72>:    or     $0x50,%eax
0xf69591ab <_ZN9Assembler4pushEP12RegisterImpl+75>:    mov    0x8(%edx),%ecx
0xf69591ae <_ZN9Assembler4pushEP12RegisterImpl+78>:    mov    %al,(%ecx)

'this' is loaded into register 'edi' and 'src' is loaded into register
'esi'. 'src' (i.e. 'esi') is compared against '0x7' which comes from
the call to 'is_valid()'. If everything is fine, we proceed at address
'0xf69591a3'. There we load the '_code_section' field with offest 0xc
from the AbstractAssembler which is in register 'edi' and stroe it in
register 'edx'. We OR the register encoding with '0x50' which succeeds
altough the actual RegisterImpl pointer (i.e. 'src') is NULL.

We then load the '_end' field with offest '0x8' from the CodeSection
in register 'edx' into 'ecx'. But this gives us a wrong address (i.e.
0xcccccccc) and that's the reason why we crash when we try to write
(i.e. emit_int8()) the register encoding from register 'al' to that
address in 'ecx'.

So the problem is that for some reason the CodeSection of the
AbstractAssembler isn't initialized correctly. This may be related to
the "codeBuffer.hpp:181:51: runtime error: member access within null
pointer of type 'struct CodeSection'" - problem when compiling with
"-fsanitize=undefined" as reported by you. I'll take a look at that
tomorrow.

Regards,
Volker




On Tue, Apr 22, 2014 at 6:34 PM, Omair Majid <omajid at redhat.com> wrote:
> * Volker Simonis <volker.simonis at gmail.com> [2014-04-22 04:41]:
>> I think the simplest and safest fix would be to make encoding() (and
>> all the corresponding functions) static functions which take a
>> Register as argument like:
>>
>> static int encoding(const RegisterImpl* r) { assert(is_valid(r),
>> "invalid register"); return (intptr_t)r; }
>>
>> This wouldn't waste any more memory and it would be fully C++
>> compliant at the price of a slightly more verbose usage:
>>
>> 2577    void Assembler::push(Register src) {
>> 2578      int encode = prefix_and_encode(RegisterImpl::encoding(src));
>> 2579
>>
>> And of course this would work with any compiler.
>
> Are you sure about this?
>
> It seems to me that a pointer created from an int does not satisfy the
> conditions for a "safely-derived pointer". It has implementation defined
> behaviour [1].
>
>> What do you think?
>
> That certainly works for me, but not sure for others.
>
> For the sake of safety, I would rather use a typedef for registers and a
> separate class with static methods to operate on the typedef.
>
> Thanks,
> Omair
>
> [1] http://cpp0x.centaur.ath.cx/basic.stc.dynamic.safety.html
>
> --
> PGP Key: 66484681 (http://pgp.mit.edu/)
> Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681