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

[Thomas Stüfe]

Hi Dimitry,

thanks for the review!

On Wed, Apr 8, 2015 at 2:40 PM, Dmitry Samersoff <
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.
>

Not necessarily true. See e.g.:
http://insanecoding.blogspot.in/2007/11/pathmax-simply-isnt.html. I guess
for most coding PATH_MAX it is a good start but truncation may still happen
and should be monitored.

But I chose snprintf over strncpy to avoid \0 padding, which I find
wasteful with such large a buffer.


>
> 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.
>
> ** arguments.cpp:3466
>
> It might be better to use snprintf here.
>
>
ok, will change.


> ** 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.
>
>
I guess this comes down to taste. I'd argue that my version - using
jio_snprintf - is easier to read and you have to think less about buffer
overwriters. I'd be curious what others think though.


> ** vmError.cpp:222
>
> It's better to keep original code.
>

Again, I wanted to avoid \0 padding. Not a strong argument I admit, but I
do not see a disadvantage with snprintf().


>
>
> Other changes looks good for me!
>

Thanks!

..Thomas


> -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.
>