RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans (original) (raw)
David Holmes david.holmes at oracle.com
Mon Mar 5 22:08:24 UTC 2018
- Previous message: RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans
- Next message: RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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 namelengthmax or resp. strlen(arg) >= arglengthmax, 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
- Previous message: RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans
- Next message: RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]