On Tue, 27 Aug 2024 01:09:09 GMT, Dean Long <dl...@openjdk.org> wrote:
>> David Holmes has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> more missing casts
>
> src/hotspot/share/classfile/javaClasses.cpp line 307:
>
>> 305: {
>> 306: ResourceMark rm;
>> 307: size_t utf8_len = static_cast<size_t>(length);
>
> I think there should be an assert that length is not negative, probably at
> the very beginning of this function.
Why? As I explained to Coleen this is the length obtained from a Java array.
All of the existing code relies on length being >= 0 and doesn't assert that
anywhere.
> src/hotspot/share/classfile/javaClasses.cpp line 350:
>
>> 348: // This check is too strict when the input string is not a valid UTF8.
>> 349: // For example, it may be created with arbitrary content via
>> jni_NewStringUTF.
>> 350: if (UTF8::is_legal_utf8((const unsigned char*)utf8_str,
>> strlen(utf8_str), false)) {
>
> Most of the time we use `is_legal_utf8`, we have a char* and have to cast it
> to unsigned char*. How about adding an inlined overload for
> is_legal_utf8(const char*, size_t) for conenience?
Sorry out of scope for this change.
> src/hotspot/share/classfile/javaClasses.cpp line 555:
>
>> 553: bool is_latin1 = java_lang_String::is_latin1(java_string);
>> 554:
>> 555: if (length == 0) return nullptr;
>
> Should this be checking for length <= 0? It looks like length can indeed be
> negative if UTF8::unicode_length() tries to return the length of a utf8
> string with length > 0x7fffffff.
??? length is assigned on line 552 and again comes from the length of a Java
array.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20560#discussion_r1732264231
PR Review Comment: https://git.openjdk.org/jdk/pull/20560#discussion_r1732264978
PR Review Comment: https://git.openjdk.org/jdk/pull/20560#discussion_r1732267249
