fix memory leaking caused by r.max and r.min

[arthurkiller]

No description provided.

[aviggiano]

Interesting! Is it possible to add a test to catch this? I'm not sure if the ones that use valgrind are 100% accurate now that you found this

[lemire]

@aviggiano Valgrind should catch all memory leaks. It would be interesting to find out that it does not.

[arthurkiller]

I found this via `leaks` tool. Have an awesome day ✉️✉️✉️✉️✉️✉️✉️✉️✉️✉️✉️✉️ Arthur lee Sent from my iPhone

…

在 2021年7月13日，下午9:07，Daniel Lemire ***@***.***> 写道：  @aviggiano Valgrind should catch all memory leaks. It would be interesting to find out that it does not. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[arthurkiller]

leaks is an OSX built-in tool and very easy to use.

leaks(1)                  BSD General Commands Manual                 leaks(1)

NAME
     leaks -- Search a process's memory for unreferenced malloc buffers

SYNOPSIS
     leaks [options] pid | partial-executable-name | memory-graph-file
     leaks [options] -atExit -- command

you can just run this on your command line

leaks redis-server-pid

To repeat the memory leak, you can try this command after call r.max r.min for a while, then you will get leaks report like this:

Process:         redis-server [47091]
Path:            /Users/USER/*/redis-server
Load Address:    0x1081a9000
Identifier:      redis-server
Version:         ???
Code Type:       X86-64
Platform:        macOS
Parent Process:  fish [46401]

Date/Time:       2021-07-14 15:23:45.713 +0800
Launch Time:     2021-07-14 15:22:41.666 +0800
OS Version:      macOS 11.1 (20C69)
Report Version:  7
Analysis Tool:   /usr/bin/leaks

Physical footprint:         6408K
Physical footprint (peak):  8312K
----

leaks Report Version: 4.0
Process 47091: 28333 nodes malloced for 29289 KB
Process 47091: 15 leaks for 1248 total leaked bytes.

    15 (1.22K) << TOTAL >>

      2 (160 bytes) ROOT LEAK: 0x7fbba0f60280 [128]
         1 (32 bytes) 0x7fbba0c127b0 [32]

      2 (160 bytes) ROOT LEAK: 0x7fbba0f60300 [128]
         1 (32 bytes) 0x7fbba7b040b0 [32]

      2 (160 bytes) ROOT LEAK: 0x7fbba39205a0 [128]
         1 (32 bytes) 0x7fbba4504240 [32]

      2 (160 bytes) ROOT LEAK: 0x7fbba3920620 [128]
         1 (32 bytes) 0x7fbba4504100 [32]

      2 (160 bytes) ROOT LEAK: 0x7fbba4504140 [128]
         1 (32 bytes) 0x7fbba3912e90 [32]

      2 (160 bytes) ROOT LEAK: 0x7fbba45041c0 [128]
         1 (32 bytes) 0x7fbba45040b0 [32]

      2 (160 bytes) ROOT LEAK: 0x7fbba4704100 [128]
         1 (32 bytes) 0x7fbba0f5e530 [32]

      1 (128 bytes) ROOT LEAK: 0x7fbba4704080 [128]

[arthurkiller]

I'm not sure why you guys did not found this leak by Valgrind, and leaks is also new to me.

But after I added a call to RedisModule_AutoMemory, everything works well 😁

[aviggiano]

Cool, I didn't know about it.
In any case, it's much more probable that our Valgrind tests are not 100% correct, so I'll have to take a look at them. I'll just add some tests to catch this issue before merging this PR

[aviggiano]

Hi there
I just figured that the valgrind tests were disabled due to an old redis issue that's already been fixed.

In any case, after re-enabling it, I wasn't able to reproduce this leak, even though it is quite obvious that RedisModule_AutoMemory is missing here. I believe that, in order to reproduce it, there must be a specific sequence of commands that will lose the pointer reference.

The docs explain that auto memory eliminates the need of calling RedisModule_CloseKey, RedisModule_FreeCallReply and RedisModule_FreeString, so probably it has to do with creating a roaring bitmap, then calling max/min then deleting it??? Not sure...

If you have any ideas (or a sequence of commands to reproduce the bug) I'm happy to add more tests. Otherwise, I'll just merge and add a new issue to make the tests more robust

[arthurkiller]

The docs explain that auto memory eliminates the need of calling RedisModule_CloseKey, RedisModule_FreeCallReply and RedisModule_FreeString, so probably it has to do with creating a roaring bitmap, then calling max/min then deleting it??? Not sure...

LGTM,

/* Mark an object as freed in the auto release queue, so that users can still
 * free things manually if they want.
 *
 * The function returns 1 if the object was actually found in the auto memory
 * pool, otherwise 0 is returned. */
int autoMemoryFreed(RedisModuleCtx *ctx, int type, void *ptr) {
    if (!(ctx->flags & REDISMODULE_CTX_AUTO_MEMORY)) return 0;

... ...

}

And I have also investigated in RedisModule_AutoMemory. AFAIK, if this is option is switched off, module memory operation such as FreeString will not take effect, and memory free should be called manually.

[aviggiano]

Merging and opening a new issue here #90 to improve Valgrind tests