Memory leak caused by uv_tty_queue_read_line

[fpiette]

Hello,

I probably found a bug in libuv TTY handling: the last buffer allocated to read lines from TTY is never freed. It is a memory leak.

I wrote a simple program to demonstrate the bug. See attached zip file for source code. I'm using MSVC2010 and run on Windows.

In the simple sample program, the 4th allocated memory block which is allocated in uv_tty_queue_read_line, is never freed. This memory block is used to read a line. And it is freed in read_cb. A new block is allocated for each new read. The issue is that read_cb is never called when uv_close(&tty) is called. The line in uv_tty_queue_read_line which allocate that memory block is: handle->alloc_cb((uv_handle_t*) handle, 8192, &handle->tty.rd.read_line_buffer);

To reproduce the issue, run the application, then type "quit<RETURN>". There are messages for each allocated and freed memory block. At the end of program, the list of not freed block are displayed. The output of the program is (In bold is what the user type):

--- AddBlock ID=1 Addr=0x5b2380 Size=16 --- AddBlock ID=2 Addr=0x5b2420 Size=64 --- AddBlock ID=3 Addr=0x5b7380 Size=8192 quit --- DelBlock ID=3 Addr=0x5b7380 Size=8192 --- AddBlock ID=4 Addr=0x5b7380 Size=8192 Walk: Closing handle 0x13fa59d60 Flags 0x1212089 Walk: Closing handle 0x13fa59d60 Flags 0x1212089 --- DelBlock ID=2 Addr=0x5b2420 Size=64 --- DelBlock ID=1 Addr=0x5b2380 Size=16 ... There is currently some memory block not freed: ... ID=4 Addr=0x5b7380 Size=8192_

The last line show that memory block 4 is still allocated. That's the memory leak. Put a breakpoint in mem_add_block, with condition "mem_count==4" and examine call stack to see where the block is allocated. It is in uv_tty_queue_read_line.

To help finding memory leaks, I wrote a very simple set of function replacing the standard memory allocation functions (Source code provided in the attached zip file). In the start of the sample program, I call uv_replace_allocator to plug my functions. At the end of program, I call mem_print_list which show all memory block allocated but not freed. Each memory block is given an ID and a conditional breakpoint in the application will show where it is allocated.

btw 1: I use MSVC2010 and Windows 7. I downloaded libuv from github on april 22, 2019.

btw2: There is a second issue with this sample program. uv_walk pass wrong handles. I create a second issue for that problem.

Regards, François Piette ttytest.zip

[neoxic]

Can you please provide a concise code snippet that depends only on libuv and demonstrates an issue without extraneous unrelated code that can also contain bugs?

[fpiette]

This is already a small test program (Less than 100 lines not counting comments) I wrote out of bigger code. The extraneous code (memory manager) you talk is necessary to demonstrate the memory leak. This code is a replacement for standard memory allocation function. You can simply throw it away if you have a better tool to show which memory is allocated and which is freed and which is not freed when it should.

[neoxic]

Ok, what I mean by all that is that you make a test case so small and free from unrelated stuff so it becomes possible for others to possibly run it on other (non-Windows) platforms which in turn may help to localize the bug quicker. I reworked your code to be platform independent and can attest to observing a memory leak on Windows.

#include <uv.h>
#include <stdlib.h>
#include <string.h>

#ifdef _MSC_VER
#define FMT "%Iu"
#else
#define FMT "%zu"
#endif

static void *malloc_(size_t size) {
    static size_t n = 0;
    size_t *p = malloc(size + sizeof(size_t) * 2);
    if (!p) return 0;
    fprintf(stderr, "+++ [#" FMT ", size: " FMT "] +++\n", ++n, size);
    *p = n;
    *(p + 1) = size;
    return p + 2;
}

static void *calloc_(size_t count, size_t size) {
    void *p = malloc_(size *= count);
    if (p) memset(p, 0, size);
    return p;
}

static void *realloc_(void *ptr, size_t size) {
    size_t *p;
    if (!ptr) return malloc_(size);
    p = realloc((size_t *)ptr - 2, size + sizeof(size_t) * 2);
    if (!p) return 0;
    fprintf(stderr, "~~~ [#" FMT ", old size: " FMT ", new size: " FMT "] ~~~\n", *p, *(p + 1), size);
    *(p + 1) = size;
    return p + 2;
}

static void free_(void *ptr) {
    size_t *p;
    if (!ptr) return;
    p = (size_t *)ptr - 2;
    fprintf(stderr, "--- [#" FMT ", size: " FMT "] ---\n", *p, *(p + 1));
    free(p);
}

static void alloc_cb(uv_handle_t *handle, size_t size, uv_buf_t *buf) {
    *buf = uv_buf_init(malloc_(size), size);
}

static void read_cb(uv_stream_t *handle, ssize_t size, const uv_buf_t *buf) {
    printf("read_cb(%p, %d)\n", handle, (int)size);
    uv_stop(handle->loop);
    free_(buf->base);
}

static void close_cb(uv_handle_t *handle) {
    printf("close_cb(%p)\n", handle);
}

int main() {
    uv_loop_t *loop;
    uv_tty_t tty;
    uv_replace_allocator(malloc_, realloc_, calloc_, free_);
    loop = uv_default_loop();
    uv_tty_init(loop, &tty, 0, 1);
    uv_read_start((uv_stream_t *)&tty, alloc_cb, read_cb);
    uv_run(loop, UV_RUN_DEFAULT);
    uv_close((uv_handle_t *)&tty, close_cb);
    uv_run(loop, UV_RUN_DEFAULT);
    uv_loop_close(loop);
    return 0;
}

Healthy output (UNIX):

$ ./a.out
+++ [#1, size: 128] +++
<--------------------------------- Press ENTER
+++ [#2, size: 65536] +++
read_cb(0x7ffef9982e20, 4)
--- [#2, size: 65536] ---
close_cb(0x7ffef9982e20)
--- [#1, size: 128] ---

Unhealthy output (Windows):

C:\libuv>libuv7.exe
+++ [#1, size: 8] +++
+++ [#2, size: 32] +++
+++ [#3, size: 8192] +++
<--------------------------------- Press ENTER
read_cb(0029FCE8, 2)
--- [#3, size: 8192] ---
+++ [#4, size: 8192] +++  <--------------- Block never freed
close_cb(0029FCE8)
--- [#2, size: 32] ---
--- [#1, size: 8] ---

Another call to alloc_cb allocated a block that can't be freed due to a missing call to read_cb.

[neoxic]

I hope the fix for https://github.com/libuv/libuv/issues/2012 is not the culprit.

[saghul]

I'm afraid it might. Getting callbacks after calling uv_close is perfectly fine son long as they happen before close_cb is called. This is already the case with uv_write and uv_shutdown requests, they'll get their callbacks called with UV_ECANCELLED.

[neoxic]

Getting callbacks after calling uv_close is perfectly fine

...for cancelled request callbacks, yes, since it is explicitly stated in the manual. As for read callbacks, it is very important to have a properly documented defined behaviour. Currently, there is no explicit requirement for a read callback to be ready to handle a closing handle's read request (by checking uv_is_closing in every callback?) as far as I can see.

[neoxic]

Furthermore, a call to uv_close normally deactivates the handle right away, i.e. all reads are stopped immediately. So your statement doesn't feel organic with the current (undocumented) behaviour.

[saghul]

...for cancelled request callbacks, yes, since it is explicitly stated in the manual. As for read callbacks, it is very important to have a properly documented defined behaviour. Currently, there is no explicit requirement for a read callback to be ready to handle a closing handle's read request (by checking uv_is_closing in every callback?) as far as I can see.

While not explicit, this is an implicit requirement. alloc_cb and read_cb come in pairs. What should libuv do if you call uv_close in your alloc_cb?

A read callback can (and will) happen (given the right circumstances, of course) when a handle is closing, and that's ok.

[saghul]

Furthermore, a call to uv_close normally deactivates the handle right away, i.e. all reads are stopped immediately. So your statement doesn't feel organic with the current (undocumented) behaviour.

If we already allocated memory in alloc_cb, the read callback must be called in order to free it.

[saghul]

I'll look deeper into https://github.com/libuv/libuv/issues/2012, but I'm reasonably confident we'll need to revert and clarify the docs.

[neoxic]

@saghul I agree with you wholeheartedly that libuv is free to do whatever it deems necessary. One thing is that some guarantees (or the lack of such) are not documented unfortunately.

[neoxic]

While not explicit, this is an implicit requirement. alloc_cb and read_cb come in pairs. What should libuv do if you call uv_close in your alloc_cb?

This can be easily documented as undefined behaviour.

A read callback can (and will) happen (given the right circumstances, of course) when a handle is closing, and that's ok.

Well, getting a read callback after a call to uv_read_stop(), for a stream for instance, does not seem to be ok, right? And that's exactly what's happening via uv_close() - an indirect call to uv_read_stop().

[saghul]

Think of it not as data being read, but as the buffer being returned to you, because libuv had already requested it but won’t be using it. Maybe we can make the status UV_ECANCELLED to make this clearer.

What I don’t understand is why you fight this. Is a memory leak ok to you? Do you have a proposal which actually fixes the problem? Documenting this as “undefined behavior” is not really acceptable here IMHO.

[neoxic]

I think I am starting to see the core of the problem...

1. If there can be no callbacks between alloc_cb and read_cb, we can see them as one callback and in this case it is possible to guarantee that there will be no read callbacks after uv_read_stop() thus no buffer leaks are possible. This is the case for UNIX as far as I can see throughout the code. A read_cb synchronously follows an alloc_cb at the same point of code.

2. I'm not a Windows expert, so I can't attest to the same behaviour. If after a buffer has been allocated via alloc_cb, it is used for reads internally allowing other (unrelated) callbacks to fire in between, it's possible to have buffer leaks, and the last read_cb should be introduced to allow for the last buffer to be freed. This makes it impossible to provide guarantees that there will be no read callbacks after uv_read_stop().

Two completely different approaches:

[START]
alloc_cb()
<internal read>
read_cb()
[END]

and

[START]
alloc_cb()
<internal read>
[END]
...
[START]
<internal read>
[END]
...
[START]
read_cb()
[END]

The ball is yours now.

[neoxic]

I certainly don't fight it as there's enough fight in this world already, and I wouldn't want to multiply our suffering even more. :-)

I think we might be having a little misunderstanding because the content of another unrelated issue (https://github.com/libuv/libuv/issues/2012) was brought into the context. That issue was based on the premise that there can be no read callbacks after uv_read_stop() (or uv_close() for that matter). And that in turn is essentially based on the first approach which is described above.

Please see another code snippet:

#include <uv.h>
#include <stdlib.h>

static void alloc_cb(uv_handle_t *handle, size_t size, uv_buf_t *buf) {
    printf("alloc_cb(%p, %d)\n", handle, (int)size);
    *buf = uv_buf_init(malloc(size), size);
}

static void read_cb(uv_stream_t *handle, ssize_t size, const uv_buf_t *buf) {
    printf("read_cb(%p, %d)\n", handle, (int)size);
    uv_stop(handle->loop);
    free(buf->base);
}

static void close_cb(uv_handle_t *handle) {
    printf("close_cb(%p)\n", handle);
}

int main() {
    uv_loop_t *loop = uv_default_loop();
    uv_tty_t tty;
    uv_tty_init(loop, &tty, 0, 1);
    uv_read_start((uv_stream_t *)&tty, alloc_cb, read_cb);
    uv_run(loop, UV_RUN_DEFAULT);
    printf("closing...\n");
    uv_close((uv_handle_t *)&tty, close_cb);
    uv_run(loop, UV_RUN_DEFAULT);
    uv_loop_close(loop);
    return 0;
}

Output on UNIX:

$ ./a.out
<------------------------------------ Press ENTER
alloc_cb(0x7ffee11bc5e0, 65536)
read_cb(0x7ffee11bc5e0, 1)
closing...
close_cb(0x7ffee11bc5e0)

Output on Windows:

C:\libuv>libuv9.exe
alloc_cb(005FF924, 8192)
<------------------------------------ Press ENTER
read_cb(005FF924, 2)
alloc_cb(005FF924, 8192)
closing...
close_cb(005FF924)

This clearly shows that on UNIX alloc_cb is called after data becomes available in the kernel and that read_cb is called synchronously.

On the other hand, we can see that on Windows alloc_cb is called before data becomes available in the kernel and that read_cb is called asynchronously.

Two completely different approaches to allocating buffers. The first approach allows to think that uv_read_stop() really stops read callbacks from happening. The second approach is incompatible with that. In other words, uv_read_stop() doesn't really mean that read callbacks will stop. If this behaviour is intended, it is very unfortunate to say the least because there is no way to really tell when it is safe to assume that there will be no read callbacks. It is a tremendous problem when we deal with managed objects, let's say, when a higher level language is used. To make a managed callback happen, it must be first prevented from being garbage collected. To undo that, we must know when it is safe to do the opposite thing which now seems to be impossible.

I hope you can see the real problem now.

[neoxic]

One possible solution on Windows would be to call a final read callback (the one that returns the allocated buffer) in uv_read_stop() synchronously. This will at least fix the leak and not introduce a new problem when uv_read_stop() doesn't really stop read callbacks from happening.

[saghul]

2\. I'm not a Windows expert, so I can't attest to the same behaviour. If after a buffer has been allocated via `alloc_cb`, it is used for reads internally allowing other (unrelated) callbacks to fire in between, it's possible to have buffer leaks, and the last `read_cb` should be introduced to allow for the last buffer to be freed. This makes it impossible to provide guarantees that there will be no read callbacks after `uv_read_stop()`.

THis is the case, IIRC. I haven't touched that bit of code in a while, but I do know alloc_cb / read_cb need not come one after another on Windows.

One possible solution on Windows would be to call a final read callback (the one that returns the allocated buffer) in uv_read_stop() synchronously. This will at least fix the leak and not introduce a new problem when uv_read_stop() doesn't really stop read callbacks from happening.

This makes sense!

Now, while looking briefly into this, I realized we have been living dangerously for a while: if one doesn't allocate a buffer in alloc_cb and calls uv_read_stop then this will crash: https://github.com/libuv/libuv/blob/e797163e754c871eceeda4a277fe8d819c06f03c/src/unix/stream.c#L1141 and if uv_close is called, this assert will trip: https://github.com/libuv/libuv/blob/e797163e754c871eceeda4a277fe8d819c06f03c/src/unix/stream.c#L1146

Looks like the rabbithole is deeper than I initially thought. 😢

[neoxic]

Well, personally, I wouldn't bother about those two. Documenting a note that any handle manipulation in alloc_cb is undefined behaviour would be sufficient although from other bug reports, I know that libuv's policy is to babysit the user. Mind you that a similar situation when undefined behaviour is properly documented is when a read_cb reports an error and libuv demands that the handle be closed by the user.