Leak in nfs_mount_8_cb() when called as a consequence of nfs_destroy_context()

[earlchew]

A very interesting case. A call to nfs_destroy_context() causes PDUs to be dequeued with RPC_STATUS_CANCEL, but nfs_mount_8_cb attempts to reconnect:

finished:
         ...
        rpc_disconnect(rpc, "normal disconnect");

        if (rpc_connect_program_async(nfs->rpc, nfs->server, NFS_PROGRAM, NFS_V3, nfs_mount_9_cb, data) != 0) {
                data->cb(-ENOMEM, nfs, command_data, data->private_data);
                free_nfs_cb_data(data);
                return;
        }

==21843== 238 (32 direct, 206 indirect) bytes in 1 blocks are definitely lost in loss record 608 of 1,534
==21843==    at 0x4C2CC4B: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21843==    by 0x916969F: rpc_connect_program_async (libnfs.c:796)
==21843==    by 0x9169DDF: nfs_mount_8_cb (libnfs.c:1090)
==21843==    by 0x9161C32: rpc_destroy_context (init.c:310)
==21843==    by 0x9168FAA: nfs_destroy_context (libnfs.c:540)
==21843==    by 0x8F3B740: ??? (in /usr/lib/python2.7/dist-packages/libnfs/_libnfs.so)
==21843==    by 0x49D189: call_function.9501 (ceval.c:4025)
==21843==    by 0x48BDDB: PyEval_EvalFrameEx.constprop.14 (ceval.c:2669)
==21843==    by 0x49D58C: call_function.9501 (ceval.c:4111)
==21843==    by 0x48BDDB: PyEval_EvalFrameEx.constprop.14 (ceval.c:2669)
==21843==    by 0x49C617: PyEval_EvalCodeEx (ceval.c:3257)
==21843==    by 0x4E6A0F: function_call (funcobject.c:526)

[earlchew]

@sahlberg Is this the right approach?

diff --git a/lib/libnfs.c b/lib/libnfs.c
index af51a90..ae8fea2 100755
--- a/lib/libnfs.c
+++ b/lib/libnfs.c
@@ -1067,6 +1067,12 @@ finished:

        rpc_disconnect(rpc, "normal disconnect");

+        if (status == RPC_STATUS_CANCEL) {
+                data->cb(-EINTR, nfs, "Command was cancelled", data->private_data);
+                free_nfs_cb_data(data);
+                return;
+        }
+
        if (rpc_connect_program_async(nfs->rpc, nfs->server, NFS_PROGRAM, NFS_V3, nfs_mount_9_cb, data) != 0) {
                data->cb(-ENOMEM, nfs, command_data, data->private_data);
                free_nfs_cb_data(data);

[sahlberg]

That looks right. Perhaps also add a check for RPC_STATUS_TiMEOUT

Awesome work and many thanks for helping this library get better.

[earlchew]

@sahlberg RPC_STATUS_TiMEOUT ? Do you mean RPC_STATUS_ERROR ?

[sahlberg]

Added just now in b5c5d59 Working on this is the reason why I have been so slow in merging your other patches.

…

On Mon, May 8, 2017 at 9:37 PM, earlchew ***@***.***> wrote: @sahlberg <https://github.com/sahlberg> RPC_STATUS_TiMEOUT ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#200 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAeNkFKcWutQXyktP6RSPTxHlwJouQWBks5r3-2lgaJpZM4NUzHI> .

[earlchew]

@sahlberg Hmm ... and what about RPC_STATUS_ERROR ?

[earlchew]

@sahlberg I'm thinking this patch is not entirely correct because nfs_mount_12_cb() is called more than once.

If any one of the callbacks gets RPC_STATUS_ERROR or RPC_STATUS_CANCEL, the entire batch should probably fail with the same error.