RFR(M): 8140482: Various minor code improvements (runtime)

[Lindenmaier, Goetz]

Hi Serguei, 

That's great, thanks a lot!

Best regards,
  Goetz.

-----Original Message-----
From: serguei.spitsyn at oracle.com [mailto:serguei.spitsyn at oracle.com] 
Sent: Monday, November 09, 2015 9:30 PM
To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Dmitry Samersoff <dmitry.samersoff at oracle.com>; David Holmes <david.holmes at oracle.com>; 'hotspot-runtime-dev at openjdk.java.net' <hotspot-runtime-dev at openjdk.java.net>
Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime)

Hi Goetz,

I'll push the fix.

Thanks,
Serguei

On 11/9/15 06:11, Lindenmaier, Goetz wrote:
> Hi David, Dmitry and Serguei,
>
> I think this is well reviewed now.
> Coleen had volunteered to sponsor this, but she is off currently I think.
> Could one of you be so friendly to sponsor this change?
>
> Thanks a lot,
>    Goetz.
>
>> -----Original Message-----
>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>> Sent: Montag, 9. November 2015 11:25
>> To: Lindenmaier, Goetz; 'hotspot-runtime-dev at openjdk.java.net'
>> Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime)
>>
>> Goetz,
>>
>> Thumbs up.
>>
>> -Dmitry
>>
>> On 2015-11-09 13:21, Lindenmaier, Goetz wrote:
>>> Oh, sure, this is to be removed.
>>> I updated the webrev in-place.
>>> http://cr.openjdk.java.net/~goetz/webrevs/8140482-covRt/webrev.04/
>>>
>>> Best regards,
>>>    Goetz.
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>>>> Sent: Freitag, 6. November 2015 18:56
>>>> To: Lindenmaier, Goetz; 'hotspot-runtime-dev at openjdk.java.net'
>>>> Subject: Re: RFR(M): 8140482: Various minor code improvements
>> (runtime)
>>>> Goetz,
>>>>
>>>> Looks good for me except one small nit:
>>>>
>>>> os_linux.cpp: 5937 is not necessary.
>>>>
>>>> -Dmitry
>>>>
>>>> On 2015-11-06 19:18, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>> Thanks for looking at this again!
>>>>> New webrev:
>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8140482-covRt/webrev.04/
>>>>>
>>>>> I copied out the remaining issues,
>>>>> Hope this makes the mail better readable:
>>>>>
>>>>>>>> 2. ps_core.c
>>>>>> If you don't trust ELF header, it might be better to write
>>>>>>
>>>>>> 822 interp_name[exec_php->p_filesz] = '\0';
>>>>> Done.
>>>>> I figured the function is indented by 3, I fixed that, too.
>>>>>
>>>>>>>> os_linux.cpp
>>>>>> It might be better to further refactor this code to avoid scanner
>>>> complains.
>>>>> Yes that's a good idea.  I also return early if the file was not opened,
>>>>> and only do core_pattern[ret] = '\0'; in the else branch.
>>>>>
>>>>>>>> xmlstream.cpp:
>>>>>>>>
>>>>>>>> 354:
>>>>>> Could you add
>>>>>>   int format_len == strlen(format);
>>>>>> and then use memcpy and calculated offsets/sizes here instead of
>> strn* ?
>>>>> I optimized it. One strlen is sufficient.
>>>>>
>>>>> I also ran another scan, which is fine.
>>>>>
>>>>> Best regards,
>>>>>    Goetz.
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>>>>>> Sent: Freitag, 6. November 2015 11:27
>>>>>> To: Lindenmaier, Goetz; 'hotspot-runtime-dev at openjdk.java.net'
>>>>>> Subject: Re: RFR(M): 8140482: Various minor code improvements
>>>> (runtime)
>>>>>> Goetz,
>>>>>>
>>>>>> Thank you for this work.
>>>>>>
>>>>>> Please, see below inline.
>>>>>>
>>>>>>
>>>>>> On 2015-11-06 00:36, Lindenmaier, Goetz wrote:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> Finally I have a new webrev:
>>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8140482-
>> covRt/webrev.03/
>>>>>>> I ran the scan again, twice, which take a while.  See coments below.
>>>>>>>
>>>>>>>> 1. libproc_impl.c
>>>>>>>>
>>>>>>>> it might be better to check that strlen(alt_root) + strlen(name) is less
>>>>>>>> than PATH_MAX at ll. 50 and return an error. Then we can safely
>> leave
>>>>>>>> strcat staff as it is.
>>>>>>> Yes, that would look better, but the scan does not grok it. It still
>>>>>>> complains: "You might overrun the 4097 byte fixed-size string alt_path
>>>> by
>>>>>> copying s without checking the length."
>>>>>>> I would like to keep my changes.
>>>>>> OK.
>>>>>>
>>>>>>>> 2. ps_core.c
>>>>>>>>
>>>>>>>> if interpreter name is truncated for some reason, we have no chance
>> to
>>>>>>>> open it and can't continue.
>>>>>>>>
>>>>>>>> So it might be better to check if exec_php->p_filesz > BUF_SIZE and
>>>>>>>> return an error immediately.
>>>>>>>>
>>>>>>>> Also, please add a comment that BUF_SIZE is (PATH_MAX +
>>>> NAME_MAX
>>>>>> + 1)
>>>>>>> I added what you asked for.  But that does not solve my issue. If there
>> is
>>>>>>> no nul in ph->core->exec_fd the code fails.  So I also left my code in
>>>> there.
>>>>>> If you don't trust ELF header, it might be better to write
>>>>>>
>>>>>> 822 interp_name[exec_php->p_filesz] = '\0';
>>>>>>
>>>>>>
>>>>>>>> 3. os_linux.cpp
>>>>>>>>
>>>>>>>> 4257:
>>>>>>>>
>>>>>>>> I'm OK with this change but we should revisit the use of
>> MAXSIGNUM
>>>>>> under
>>>>>>>> Linux and probably define it to _NSIG for the rest of VM
>>>>>>> I opened an extra issue for this (for the change in SR_initialize() I
>>>> removed.)
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8141529
>>>>>> OK
>>>>>>
>>>>>>>> 5944:
>>>>>>>>
>>>>>>>> What is the reason of the change? core_pattern is initialized to zero
>> at
>>>>>>>> ll. 5939.
>>>>>>> If the file is corrupted, there might be more than 128 chars in it.
>>>>>>>
>>>>>>>> It might be better to check that ret is less than corep_pattern_len
>> and
>>>>>>>> then use ret instead of strlen at ll. 5948
>>>>>>> That's a good point.  It also saves the cost of strlen(). I now return if
>>>>>>> too many chars were read.
>>>>>>> But with removing writing \'0' the scan now complains, again:
>>>>>> "string_null_argument: Function read does not terminate string
>>>>>> *core_pattern.",
>>>>>>> And further down at the next strlen: "Passing unterminated string
>>>>>> core_pattern to strlen, which expects a null-terminated string."
>>>>>>> I'd like to add the \'0' again.
>>>>>> It might be better to further refactor this code to avoid scanner
>>>> complains.
>>>>>> 5937:
>>>>>>
>>>>>> if (ret <= 0 || ret >= core_pattern_len || *core_pattern == '\n') {
>>>>>>     return -1;
>>>>>> }
>>>>>>
>>>>>> after that ifs at 5942 and 5991 is not necessary and we can make
>> scanner
>>>>>> happy as
>>>>>>
>>>>>> 5943
>>>>>>
>>>>>> core_pattern[ret] = '\0';
>>>>>> if (core_pattern[ret-1] == '\n') core_pattern[ret-1] = '\0'
>>>>>>
>>>>>>
>>>>>>>> deoptimization.cpp:
>>>>>>>>
>>>>>>>> 2085, 2182:
>>>>>>>>
>>>>>>>> jio_snprintf always return -1 and always null-terminate string in case
>>>>>>>> of overflow.  So original code is incorrect and we can just remove it.
>>>>>>> Good point.  I think I fixed jio_snprintf at some point :) Should have
>>>> known.
>>>>>> OK.
>>>>>>
>>>>>> PS:
>>>>>> Be careful fixing jio_snprintf because posix versions of vsnprintf
>>>>>> returns the number of charаcters to be written but windows version
>>>>>> returns -1 in case of overflow
>>>>>>
>>>>>> Correct solution that uses vsnptrinf(NULL, 0 ... ) and _vscprintf to
>>>>>> check overflow explicitly obviously will have performance impact.
>>>>>>
>>>>>>
>>>>>>>> attachListener.hpp:
>>>>>>>>
>>>>>>>> 125,143:
>>>>>>>>
>>>>>>>> It might be better to calculate strlen(name) once, than use memcpy.
>>>>>>> Fixed.
>>>>>> OK.
>>>>>>
>>>>>>>> heapDumper.cpp:
>>>>>>>>
>>>>>>>> 1983:
>>>>>>>>
>>>>>>>>     we checked that HeapDumpPath fits in the buffer at ll 1973, so we
>> can
>>>>>>>> safely use strcpy() here.
>>>>>>> I had removed my fix as David asked for it. I change it to strcpy.
>>>>>> OK.
>>>>>>
>>>>>>>> xmlstream.cpp:
>>>>>>>>
>>>>>>>> 354:
>>>>>>>>
>>>>>>>>     What is the reason of this change? we checked that string fits to
>>>>>>>> buffer at ll 348.
>>>>>>> A guarantee can be overruled with SuppressErrorAt. Also in the opt
>>>> build.
>>>>>>> So the code is also reachable with bad values.
>>>>>> This code is executed in a loop so I would prefer to don't have extra
>>>>>> strlen calls here.
>>>>>>
>>>>>> Could you add
>>>>>>
>>>>>>   int format_len == strlen(format);
>>>>>>
>>>>>> and then use memcpy and calculated offsets/sizes here instead of
>> strn* ?
>>>>>> -Dmitry
>>>>>>
>>>>>>
>>>>>>> Best regards,
>>>>>>>    Goetz.
>>>>>>>
>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2015-10-27 10:39, Lindenmaier, Goetz wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> SAP requires us to fix a row of issues in hotspot.  I'd like to share
>> these
>>>>>>>>> with openjdk:
>>>>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8140482-
>>>> covRt/webrev.00
>>>>>>>>> Please review this change.  I please need a sponsor.
>>>>>>>>>
>>>>>>>>> The fixes in detail:
>>>>>>>>>
>>>>>>>>> libproc_impl.c:
>>>>>>>>> Do strncpy in case getenv returned a bad string.
>>>>>>>>> Strcat could overflow the buffer. Use strncat instead.
>>>>>>>>>
>>>>>>>>> ps_core.c:
>>>>>>>>> Pread not necessarily terminates interp_name which is printed
>>>>>> thereafter.
>>>>>>>>> Increase buffer size by 1 and add '\0'.
>>>>>>>>>
>>>>>>>>> stubRoutines_x86.cpp:
>>>>>>>>> Cast to proper type. This way, left and right of '&' have the same
>> type.
>>>>>>>>> attachListener_linux.cpp:
>>>>>>>>> Read does not terminate buf. Size for '\0' is already considered.
>>>>>>>>>
>>>>>>>>> os_linux.cpp:
>>>>>>>>> Array sigflags[] has size MAXSIGNUM==32.  _NSIG is bigger than
>>>>>>>>> MAXSIGNUM (_NSIG == 65 on my machine).
>>>>>>>>> sig is checked to be smaller than _NSIG. Later, in set_our_sigflags(),
>>>>>>>>> sig is used to access sigflags[MAXSIGNUM] which can overflow the
>>>> array.
>>>>>>>>> Should we also increase MAXSIGNUM?
>>>>>>>>> os::get_core_path(): read does not terminate string, but strlen is
>>>>>>>>> called on it.  The size already foresees one char for the '\0' byte.
>>>>>>>>>
>>>>>>>>> codeBuffer.cpp:
>>>>>>>>> New_capacity is not initialized. Figure_expanded_capacities()
>> handles
>>>>>> this
>>>>>>>>> correctly, but initializing this is cheap and safe.
>>>>>>>>>
>>>>>>>>> dict.cpp:
>>>>>>>>> If j-- is executed for j==0, the loop aborts because j is unsigned (0--
>>> =
>>>> b-
>>>>>>>>> _cnt).
>>>>>>>>> Instead, only do j++ if necessary.
>>>>>>>>>
>>>>>>>>> generateOopMap.cpp:
>>>>>>>>> Idx is read from String. This is only called with constant strings, so
>>>>>> compare
>>>>>>>>> should be folded away by optimizing compilers if inlined.
>>>>>>>>>
>>>>>>>>> deoptimization.cpp:
>>>>>>>>> If buflen == 0, buf[-1] is accessed.
>>>>>>>>>
>>>>>>>>> task.cpp:
>>>>>>>>> Fatal can return if -XX:SuppressErrorAt is used. Just don't access the
>>>>>>>>> array in this case.
>>>>>>>>>
>>>>>>>>> attachListener.hpp:
>>>>>>>>> Do strncpy to not overflow buffer. Don't write more chars than
>>>> before.
>>>>>>>>> heapDumper.cpp:
>>>>>>>>> strncpy does not null terminate.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Some of these, as the issue in codeBuffer.cpp, are actually handled
>>>>>>>> correctly.
>>>>>>>>> Nevertheless this is not that obvious so that somebody changing
>> the
>>>>>> code
>>>>>>>>> Could oversee he has to add the initialization.
>>>>>>>>>
>>>>>>>>> Some of these fixes are part of SAP JVM for a long time.  This
>> change
>>>> has
>>>>>>>>> been tested with our nightly build of openJDK.
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>>    Goetz,.
>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Dmitry Samersoff
>>>>>>>> Oracle Java development team, Saint Petersburg, Russia
>>>>>>>> * I would love to change the world, but they won't give me the
>>>> sources.
>>>>>>
>>>>>> --
>>>>>> Dmitry Samersoff
>>>>>> Oracle Java development team, Saint Petersburg, Russia
>>>>>> * I would love to change the world, but they won't give me the
>> sources.
>>>>
>>>> --
>>>> Dmitry Samersoff
>>>> Oracle Java development team, Saint Petersburg, Russia
>>>> * I would love to change the world, but they won't give me the sources.
>>
>> --
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.