RFR(xs): 8076475: Misuses of strncpy/strncat
Thomas Stüfe
thomas.stuefe at gmail.com
Fri Apr 10 11:25:11 UTC 2015
.. 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/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.
>
More information about the hotspot-runtime-dev
mailing list