RFR: 8312401: SymbolTable::do_add_if_needed hang when requesting length exceeds max_symbol_length [v2]

[Jiangli Zhou]

On Sat, 22 Jul 2023 00:39:19 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>> Please review the simple fix to resolve infinite loop in SymbolTable::do_add_if_needed caused by extra long symbol string that exceeds Symbol::max_length(). See JDK-8312401 for details.
>> 
>> The jtreg test is converted from a test case constructed by @cushon.
>
> Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update according to David Holmes' comments.
>   - In java_lang_String::as_symbol and java_lang_String::as_symbol_or_null, check input string length and truncate to max symbol length if exceeding.
>   - Change java_lang_Throwable::create_initialization_error to avoid using Symbol for message.

> > I considered changing to use char * for the message in java_lang_Throwable::create_initialization_error ([e2223f0](https://github.com/openjdk/jdk/commit/e2223f0613d53e89e11af587dc3d41a25ffd6849)). But we need to handle all other callers as well. Exceptions::log_exception can use char * also. The other callers still require Symbol.
> 
> Hmmm ... by using a Symbol we are artificially restricting the length of the exception message. That is fine if the VM is constructing the message, but not if the message can come from an external source as in this case. IIUC only `java_lang_Throwable::create_initialization_error` tries to use an existing, externally defined message, so only it should have this problem. So we either stop using a Symbol in this case (we copied the code from the resolution error case) or we handle the limitation - which to me means we should truncate so that we at least retain most of the potentially useful information. 

Those are good points. I've updated the PR to use `char *` for message in java_lang_Throwable::create_initialization_error. 

I also changed java_lang_String::as_symbol and java_lang_String::as_symbol_or_null to check input string length and truncate to max symbol length if exceeding. With the fix in java_lang_Throwable::create_initialization_error, that would mostly be a no-op, but just to be safe. I also added asserts in SymbolTable::new_symbol to check length.

> It seems unrealistic to have exception messages this large in non-tests. I'm not sure why we use Symbol here in the first place. @coleenp will know but is away this week.

I'll wait for @coleenp's review/feedback.

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

PR Comment: https://git.openjdk.org/jdk/pull/14938#issuecomment-1646366511