RFR: JDK-8261791: handleSendFailed in SctpChannelImpl.c potential leaks

[Chris Hegarty]

On Tue, 16 Feb 2021 12:26:54 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:

> In another bug  this question from me  was answered by  Alan Bateman :
> 
> Btw. while adjusting Java_sun_nio_ch_sctp_SctpChannelImpl_receive0 , I started to wonder what happens to the allocated memory in the same file in handleSendFailed ( if ((addressP = malloc(dataLength)) == NULL) ) in early return cases incl. the CHECK_NULL , is there some deallocation missing there too ?
> 
> 
> Yes, the error paths in handleSendFailed should be looked at. If NewDirectByteBuffer or recvmsg fails then addressP needs to be freed. Furthermore, if the NewObject fails and bufferObj != NULL then the memory for the direct buffer will need to be freed too (as JNI NewDirectByteBuffer does not setup a cleaner).
> 
> 
> So I added freeing of the malloced memory to handleSendFailed .
> Please review !
> 
> Thanks, Matthias

Marked as reviewed by chegar (Reviewer).

src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 236:

> 234:             return;
> 235:         }
> 236: 

This looks fine. If NewDirectByteBuffer returns NULL there will be a pending OutOfMemoryError.

src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 251:

> 249:                 free(addressP);
> 250:                 handleSocketError(env, errno);
> 251:                 return;

At this point the direct ByteBuffer has been successfully allocated and refers to the memory at addressP. But the direct ByteBuffer, while in the Java heap, will not have any references to it, so freeing addressP should be fine here.

src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 257:

> 255:                 //TODO: assert false: "should not reach here";
> 256:                 free(addressP);
> 257:                 return;

same a previous comment - looks ok.

src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 270:

> 268:         return;
> 269:     }
> 270:     (*env)->SetObjectField(env, resultContainerObj, src_valueID, resultObj);

If NewObject returns NULL, there will be a pending OutOfMemoryError. Freeing addressP should be safe here, same reasoning as above.

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

PR: https://git.openjdk.java.net/jdk/pull/2586