RFR(xs): 8076475: Misuses of strncpy/strncat
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/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