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

[Lindenmaier, Goetz]

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.