RFR(xs): 8076475: Misuses of strncpy/strncat
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Apr 20 21:22:07 UTC 2015
This seems good.
Coleen
On 4/20/15, 4:58 AM, Thomas Stüfe wrote:
> 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-dev
mailing list