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

Thomas Stüfe thomas.stuefe at gmail.com
Mon Apr 20 08:58:05 UTC 2015

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