On Tue, 5 Dec 2023 23:32:41 GMT, Kevin Rushforth <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> check return value of JNI functions
>
> modules/javafx.graphics/src/main/native-glass/mac/GlassMacros.h line 178:
>
>> 176:
>> 177: #define GLASS_CHECK_NONNULL_EXCEPTIONALLY_RETURN(ENV, EXPR)
>> \
>> 178: GLASS_CHECK_EXCEPTION(ENV);
>> \
>
> Should this call `GLASS_CHECK_EXCEPTIONALLY_RETURN` instead? It doesn't
> matter for the current places where it is used, but it might be more
> consistent.
Yes.
> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp
> line 159:
>
>> 157: memset(wstr, 0, wstr_len * sizeof(WCHAR));
>> 158: MultiByteToWideChar(CP_UTF8, 0, str, -1, wstr, wstr_len);
>> 159: WindowsCreateString(wstr, wstr_len - 1, &hstr_);
>
> This could use some error checking. At a minimum, `wstr` should be checked
> for null and skip the rest if it is. I think you also should check `wstr_len
> == 0`, since it is will return 0 on error. You might also consider whether
> you need to check the return value of `WindowsCreateString`. You would need
> to make sure `hstr_ = nullptr` if there are any errors.
I added the checks, but not for `WindowsCreateString`, since it is specified to
set `hstr_` to null in case of failure.
> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp
> line 165:
>
>> 163: hstring::~hstring()
>> 164: {
>> 165: WindowsDeleteString(hstr_);
>
> After adding the check for a failure to allocate the string, a null check is
> needed here (unless `WindowsDeleteString` specifies that it does nothing if
> the string is null).
`WindowsDeleteString` does nothing when null is passed into it, but I added a
null check anyway so people don't stumble upon it and wonder.
> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp
> line 182:
>
>> 180: size_t len = strlen(message);
>> 181: char* msg = new char[len + 1];
>> 182: strcpy_s(msg, len + 1, message);
>
> You should check for null before copying.
Done.
> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp
> line 199:
>
>> 197: char* result = new char[message_length + error_length];
>> 198: WideCharToMultiByte(CP_ACP, 0, error, -1, result + message_length,
>> error_length, NULL, FALSE);
>> 199: memcpy_s(result, message_length, message, message_length);
>
> Same comment about error checking as in `hstring`.
Done.
> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp
> line 208:
>
>> 206: RoException::~RoException()
>> 207: {
>> 208: delete[] message_;
>
> Check `message_` for null (meaning you will need to initialize it to null in
> case of error).
I added a null check, and also initialize the field to null in the constructor.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416495255
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416486073
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416494229
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416494466
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416494701
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416495125
