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

Fri Nov 6 10:27:23 UTC 2015

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.