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

[Justin King]

On Tue, 21 Mar 2023 22:30:03 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Justin King has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update based on review
>>   
>>   Signed-off-by: Justin King <jcking at google.com>
>
> 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?

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

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