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

[Lindenmaier, Goetz]

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.