On Wed, 6 Nov 2024 15:27:16 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> src/java.base/share/native/libjava/NativeLibraries.c line 67: >> >>> 65: strcat(jniEntryName, "_"); >>> 66: strcat(jniEntryName, cname); >>> 67: } >> >> I would prefer this be directly inlined at the sole call (in >> findJniFunction), >> to make it easier to verify there aren't any buffer overrun problems. (I >> don't >> think there are, but looking at this in isolation triggered warnings in my >> head.) >> >> Also, it looks like all callers of findJniFunction ensure the cname argument >> is non-null. So there should be no need to handle the null case in >> findJniFunction (other than perhaps an assert or something). That could be >> addressed in a followup. (I've already implicitly suggested elsewhere in this >> PR revising this function in a followup because of the JNI_ON[UN]LOAD_SYMBOLS >> thing.) > > @kimbarrett I added this to https://bugs.openjdk.org/browse/JDK-8343703. You > are not as explicit here as the other places you commented that it is okay to > do as a follow-up, but I'll assume that was what you meant. If not, let me > know, and I'll look at fixing it for this PR already. The first part, eliminating the (IMO not actually helpful) helper function, I wanted done here. The second part, cleaning up or commenting the calculation of the length and dealing with perhaps unneeded conditionals, I'm okay with being in a followup. I guess I can live with the first part being in that followup too. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1831728737
