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

[David Holmes]

On 6/03/2018 3:17 AM, Chris Plummer wrote:
> Asserts imply something that is suppose to never happen, but that you 
> want to check for in debug builds to help uncover bugs. Given this, 
> either we have a bug (and someone can pass in a name that is too long), 
> or coverity is complaining about something that can never happen, or the 
> assert is invalid. So the potential fixes are:
> 
> -Fix the problem up the call chain were the invalid string can be passed 
> in.
> -Tell coverity to clam up because having the string be too long is not 
> possible.
> -Leave in your fix but remove the assert.

I hadn't looked into the calling context for this, but a too long name 
should be impossible. The allowed names come from here:

// names must be of length <= AttachOperation::name_length_max
static AttachOperationFunctionInfo funcs[] = {
   { "agentProperties",  get_agent_properties },
   { "datadump",         data_dump },
   { "dumpheap",         dump_heap },
   { "load",             load_agent },
   { "properties",       get_system_properties },
   { "threaddump",       thread_dump },
   { "inspectheap",      heap_inspection },
   { "setflag",          set_flag },
   { "printflag",        print_flag },
   { "jcmd",             jcmd },
   { NULL,               NULL }
};

and name_length_max comes from the longest defined name: agentProperties.

Further, AFAICS, set_name is only actually called on Windows. And we 
again check the incoming cmd "name" to ensure it isn't too big.

So the whole copying code seems somewhat overly conservative:
- we've limited the name to below the maximum
- we have an assert just in case someone adds a new name and forgets to 
increase the maximum (there are actually asserts at multiple levels)
- but we also copy as-if the name can be longer than expected

The irony is that the current code was put in place because of coverity!

https://bugs.openjdk.java.net/browse/JDK-8140482

Not sure why it isn't just using strncpy though.

David

> thanks,
> 
> Chris
> 
> On 3/5/18 7:37 AM, Langer, Christoph wrote:
>> Hi Thomas,
>>
>> well, I think this discussion is beyond the scope of my contribution. 
>> Probably one doesn’t want the risk of JVM crashes/exits just because 
>> someone shoots in a bad attach operation name which is too long.
>>
>> So, may I consider it reviewed from your end? I’m trying the 
>> submission repo right now with this change…
>>
>> Best regards
>> Christoph
>>
>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>> Sent: Montag, 5. März 2018 15:53
>> To: Langer, Christoph <christoph.langer at sap.com>
>> Cc: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>; 
>> serviceability-dev at openjdk.java.net
>> Subject: Re: RFR (XS): 8199010: attachListener.hpp: Fix potential null 
>> termination issue found by coverity scans
>>
>> Hi Christoph,
>>
>> Seeing that truncation is considered assertion worthy, should we 
>> really hide it in release?
>>
>> Gruß Thomas
>>
>> On Mar 5, 2018 10:03, "Langer, Christoph" 
>> <christoph.langer at sap.com<mailto:christoph.langer at sap.com>> wrote:
>> Hi,
>>
>> please review a small fix that was identified by a coverity code scan.
>>
>> In case strlen(name) was the same or larger than name_length_max or 
>> resp. strlen(arg) >= arg_length_max, the _name or _arg fields would 
>> not get null terminated correctly.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8199010
>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8199010.0/
>>
>> Thanks
>> Christoph
> 
> 
>