RFR: JDK-8304684: Memory leak in DirectivesParser::set_option_flag [v2]

[Dean Long]

On Wed, 22 Mar 2023 00:04:45 GMT, Justin King <jcking at openjdk.org> wrote:

>> src/hotspot/share/compiler/directivesParser.cpp line 351:
>> 
>>> 349:         }
>>> 350: 
>>> 351:         FREE_C_HEAP_ARRAY(char, s);
>> 
>> This looks unsafe.  We shouldn't free the memory without clearing all references to it, otherwise there is a dangling pointer.  There is already another reference to the memory because of this call:
>> 
>> `(set->*test)((void *)&s);` (see the set_function_definition macro)
>> 
>> I think it would be better to move this copying call until after validation has been done.
>
> I am very confused actually, `(set->*test)((void *)&s);` calls DirectiveSet::set_X. Looking at the code, it simply just stores the pointer? Is the DirectiveSet supposed to own the option? And if so, who is freeing it then? It doesn't look like cloning actually clones the underlying storage and the DirectiveSet destructor doesn't free it.
> 
> So really DirectiveSet::~DirectiveSet should be freeing the string storage and DirectiveSet::set_X is taking ownership. Yeah?

Right, the ownership of the string seems unclear.  If we try to free the string storage in ~DirectiveSet I think it will result in a double free, because clone() appears to do a shallow copy.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13125#discussion_r1146973287