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

[Dmitry Samersoff]

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/libproc_impl.c
>     >     ** agent/src/os/linux/libproc_impl.c
>     >
>     >     newlib->name has the size of (PATH_MAX + NAME_MAX + 1)
>     >
>     >     see ./src/os/bsd/libproc_impl.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/libproc_impl.c
>     >     > agent/src/os/linux/libproc_impl.c:
>     >     >                 - missing \0 on truncation. Replaced with
>     >     snprintf, add
>     >     > truncation handling
>     >     >
>     >     > src/os/bsd/dtrace/libjvm_db.c
>     >     > src/os/solaris/dtrace/libjvm_db.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/decoder_machO.cpp
>     >     >                 - missing \0 on truncation.
>     >     >
>     >     > src/share/vm/compiler/compilerOracle.cpp
>     >     >                 - Replaced with jio_snprintf, 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
>     jio_snprintf -
>     >     > 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.