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

[Langer, Christoph]

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