RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans

[Thomas Stüfe]

Maybe stupid question, any reason we could not just strdup() those strings?
And add an ~AttachOperation dtor to clean them up again?

Otherwise, I usually prefer using snprintf (or, jio_snprintf or the new
os::snprintf()) with "%s" as format string:

jio_snprintf(_name, sizeof(_name), "%s", inputstring).

This takes care cleanly of zero termination and truncation and one does not
have to think too hard about MIN2 expressions. One also saves the strlen()
call.

strncpy is almost completely useless, usually, because it does not zero
terminate cleanly and the \0 padding is just weird.

Best Regards, Thomas



On Tue, Mar 6, 2018 at 8:03 PM, Langer, Christoph <christoph.langer at sap.com>
wrote:

> Hi Chris,
>
> > > I don't know why strncpy would do zero padding?
> >  From the man page:
> >
> >       The stpncpy() and strncpy() functions copy at most len characters
> > from src into dst.  If src is less than len
> >       characters long, the remainder of dst is filled with `\0'
> > characters.  Otherwise, dst is not terminated.
>
> Ok, yes, it depends on the length that you specify. I meant if you always
> specify the full buffer length (name_length_max+1), it would pad. Never
> mind.
>
> > Why don't we do a restart and look at the original problem whose fix
> > introduced the current issue. Here's the original code:
> >
> >      assert(strlen(name) <= name_length_max, "exceeds maximum name
> > length");
> >      strcpy(_name, name);
> >
> > And here's how set_name is used:
> >
> > class AttachOperation: public CHeapObj<mtInternal> {
> >    enum {
> >      name_length_max = 16,       // maximum length of  name
> >    };
> >    char _name[name_length_max+1];
> > }
> >
> >    if (strlen(cmd) > AttachOperation::name_length_max) return
> > ATTACH_ERROR_ILLEGALARG;
> >    op->set_name(cmd);
> >
> > I don't see why coverity would complain about this. It can statically
> > see that there will be no buffer overflow. Does it think there are other
> > potential callers of set_name() for which no size check is made? I can't
> > find any. The only clue from JDK-8140482 is:
> >
> > attachListener.hpp:
> > Do strncpy to not overflow buffer. Don't write more chars than before.
> >
> > David, you had pointed that the strcpy complained seemed erroneous
> > during the review for JDK-8140482. The final word from Goetz was:
> >
> > > I agree that I can not find another possible issue
> > > with the strcpy.
> > > Still I think it's better to have the strncpy, as it would have
> > > protected against the bug in attachListener_windows.cpp.
> > > But if you insist I'll just remove it.
> > http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-
> > November/016363.html
> >
> > Dmitry Samersoff later added:
> > > It might be better to calculate strlen(name) once, than use memcpy.
> > So we started with what appears to be an invalid and unexplained
> > complaint by coverity about strcpy, changed it to strncpy to appease
> > coverity, changed it to memcpy to avoid two calls to strlen, and now
> > want to fix a different complaint by coverity on that solution.
> >
> > I say go back to strcpy and see if/why coverity is still complaining.
>
> Ok, agreed. I'll put the original code in place and see what coverity
> exactly says. Maybe it wants to see a guarantee instead of an assert. But
> let's see.
>
> Best regards
> Christoph
>
>