RFR(xs): 8076475: Misuses of strncpy/strncat (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Mon Apr 20 21:22:07 UTC 2015

This seems good. Coleen

On 4/20/15, 4:58 AM, Thomas Stüfe wrote:

Hi,

could I have an additional reviewer, please? current webrev is: http://cr.openjdk.java.net/~stuefe/webrevs/8076475/webrev.01/webrev/ Thank you! ..Thomas On Fri, Apr 10, 2015 at 1:25 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:

.. adding hotspot-dev to attract another reviewer.

On Fri, Apr 10, 2015 at 11:47 AM, Dmitry Samersoff <_ _dmitry.samersoff at oracle.com> wrote:

Thomas,

I can sponsor the push, but you need an official reviewer. -Dmitry On 2015-04-10 12:31, Thomas Stüfe wrote: Dmitry, Thanks for the review!

Still need a second review and also a sponsor. .. Thomas On Apr 10, 2015 10:46 AM, "Dmitry Samersoff" <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>> wrote: Thomas, Looks good for me! Thank you for doing it. -Dmitry On 2015-04-10 10:50, Thomas Stüfe wrote: > Hi Dimitry, > > see new version here: > > http://cr.openjdk.java.net/~stuefe/webrevs/8076475/webrev.01/webrev/ > > > On Wed, Apr 8, 2015 at 2:40 PM, Dmitry Samersoff > <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com> <mailto:dmitry.samersoff at oracle.com_ _<mailto:dmitry.samersoff at oracle.com>>> wrote: > > Thomas, > > ** agent/src/os/bsd/libprocimpl.c > ** agent/src/os/linux/libprocimpl.c > > newlib->name has the size of (PATHMAX + NAMEMAX + 1) > > see ./src/os/bsd/libprocimpl.h:92 > > So no truncation is actually possible here. > > To make this code nice-looking it's better to add: > > if (strlen(libname) > sizeof(newlib->name)) { > // Bail out with debug message > ... > } > strcpy(newlib->name, libname); > > rather than use snprintf here. > > > I changed the coding according to your suggestions. > > There is still the strlen vs strnlen question Kim mentioned, but we use > strlen all over the place, and I am not sure if all platforms support it > (AIX was known to have a broken strnlen implementation, one would need > to check that first before changing strlen to strnlen) > > > ** arguments.cpp:3466 > > It might be better to use snprintf here. > > > Agree, done > > > ** arguments.cpp:3629 > > It's better to just replace strncat/strncpy to strcat/strcpy > in the original code - as soon as we allocating memory with respect to > variable lenght, we don't need to check this length again. > > > Sorry, I disagree here - I think using snprintf makes the code more > readable. It comes down to a matter of taste, so lets see what others think. > > > > ** vmError.cpp:222 > > It's better to keep original code. > > > Agree, done > > Best Regards, Thomas > > > > Other changes looks good for me! > -Dmitry > > > On 2015-04-08 11:09, Thomas Stüfe wrote: > > Hi, > > > > please review these small fixes around use of strncpy/strncat. > > > > Bug report: https://bugs.openjdk.java.net/browse/JDK-8076475 > > Webrev: > > http://cr.openjdk.java.net/~stuefe/webrevs/8076475/webrev.00/webrev/ > > > > Changes in detail are: > > > > agent/src/os/bsd/libprocimpl.c > > agent/src/os/linux/libprocimpl.c: > > - missing \0 on truncation. Replaced with > snprintf, add > > truncation handling > > > > src/os/bsd/dtrace/libjvmdb.c > > src/os/solaris/dtrace/libjvmdb.c > > @@ -580,17 +580,18 @@ > > - overwrite on truncation > > @@ -1093,13 +1094,13 @@ > > - overwrite on truncation > > > > src/share/vm/compiler/compileBroker.hpp > > - missing \0 on truncation. > > > > src/share/tools/hsdis/hsdis.c > > - missing \0 > > > > src/os/bsd/vm/decodermachO.cpp > > - missing \0 on truncation. > > > > src/share/vm/compiler/compilerOracle.cpp > > - Replaced with jiosnprintf, less awkward and > does not > > restrict each part to 255 chars each. > > > > src/share/vm/compiler/disassembler.cpp > > - missing \0 on truncation. > > > > src/share/vm/runtime/arguments.cpp > > @@ -2703,11 +2703,11 @@ > > - replaced with strdup, easier to read > > @@ -3294,12 +3294,11 @@ > > - the same > > @@ -3627,18 +3626,14 @@ > > - replace strncpy/strncat sequence with jiosnprintf - > > easier to read. > > - replace malloc/strncpy with os::strdup > > > > src/share/vm/utilities/ostream.cpp > > - avoid \0 padding > > > > src/share/vm/utilities/vmError.cp > > @@ -219,7 +219,7 @@ > > - avoid \0 padding > > @@ -463,14 +463,7 @@ > > - simplified > > > > Kind regards, Thomas > > > > > -- > 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.

More information about the hotspot-dev mailing list