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

[Dmitry Samersoff]

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.