[Dovecot] Patch to fix leak in imap_refresh_proctitle in beta[5, 6]
The "imap" process of dovecot-2.0.beta[5,6] grows very large (I impose no system limits), e.g. exceeding 4.8GB on a 64-bit system. These messages appear in the logs: Warning: Growing pool 'imap client' with: 2048 Warning: Growing pool 'Cache fields' with: 2048 Warning: Growing data stack with: 32768 Warning: Growing data stack with: 65536 Warning: Growing data stack with: 131072 [...] Warning: Growing data stack with: 1073741824 Warning: Growing data stack with: 2147483648 Warning: Growing data stack with: 4294967296 Every time the data stack grows, the backtrace ends with either: 2 libdovecot.0.dylib 0x00000001059a35c3 pool_data_stack_realloc + 72 -> 3 libdovecot.0.dylib 0x0000000105994d59 buffer_alloc + 59 -> 4 libdovecot.0.dylib 0x0000000105994ef9 buffer_check_limits + 127 -> 5 libdovecot.0.dylib 0x00000001059950e8 buffer_append + 38 -> 6 imap 0x0000000105867333 imap_refresh_proctitle + 218 -> 7 imap 0x000000010585f18e client_command_input + 190 -> [...] or 2 libdovecot.0.dylib 0x00000001059a35c3 pool_data_stack_realloc + 72 -> 3 libdovecot.0.dylib 0x0000000105994d59 buffer_alloc + 59 -> 4 libdovecot.0.dylib 0x0000000105994ef9 buffer_check_limits + 127 -> 5 libdovecot.0.dylib 0x00000001059950e8 buffer_append + 38 -> 6 imap 0x0000000105867333 imap_refresh_proctitle + 218 -> 7 imap 0x00000001058666ce cmd_sync_continue + 199 -> [...] Here is a patch to fix it: --- a/src/imap/main.c (beta6) +++ b/src/imap/main.c (working copy) @@ -39,11 +39,12 @@ #define IMAP_PROCTITLE_PREFERRED_LEN 80 struct client *client; struct client_command_context *cmd; - string_t *title = t_str_new(128); + string_t *title; if (!verbose_proctitle) return; + title = str_new(default_pool, 128); str_append_c(title, '['); switch (imap_client_count) { case 0: @@ -72,6 +73,7 @@ } str_append_c(title, ']'); process_title_set(str_c(title)); + str_free(&title); } static void client_kill_idle(struct client *client) And perhaps there should be a warning when data is allocated out of the primordial data stack (as opposed to a nested one)?
On Mon, 2010-06-14 at 17:18 -0500, Mike Abbott wrote:
6 imap 0x0000000105867333 imap_refresh_proctitle + 218 -> 7 imap 0x000000010585f18e client_command_input + 190 ->
Looks ok..
6 imap 0x0000000105867333 imap_refresh_proctitle + 218 -> 7 imap 0x00000001058666ce cmd_sync_continue + 199 ->
But how does this happen? Did it optimize away some functions or have you added more imap_refresh_proctitle() calls?
And perhaps there should be a warning when data is allocated out of the primordial data stack (as opposed to a nested one)?
But it's not. It always frees the memory when returning back to ioloop. I could understand if it wasted a few kilobytes of memory, but how do you manage to make it call imap_refresh_proctitle() millions of times without returning to ioloop?
6 imap 0x0000000105867333 imap_refresh_proctitle + 218 -> 7 imap 0x00000001058666ce cmd_sync_continue + 199 ->
But how does this happen? Did it optimize away some functions
Yeah optimized out tail-calls, e.g. client_destroy -> imap_refresh_proctitle and client_command_free -> imap_refresh_proctitle. I have been digging deeper and found that sometimes imap_clients->command_queue->name points to garbage, so imap_refresh_proctitle is appending 500MB strings of garbage. Combined with a little command pipelining this leads to 4+GB data stack pools. I'll resume digging tomorrow. Let me know if you need any info; I can reproduce this in seconds.
have you added more imap_refresh_proctitle() calls?
Nope.
On Tue, 2010-06-15 at 21:04 -0500, Mike Abbott wrote:
6 imap 0x0000000105867333 imap_refresh_proctitle + 218 -> 7 imap 0x00000001058666ce cmd_sync_continue + 199 ->
But how does this happen? Did it optimize away some functions
Yeah optimized out tail-calls, e.g. client_destroy -> imap_refresh_proctitle and client_command_free -> imap_refresh_proctitle. I have been digging deeper and found that sometimes imap_clients->command_queue->name points to garbage, so imap_refresh_proctitle is appending 500MB strings of garbage.
Is it complete garbage or 0xde character? (Or if you don't use --with-devel-checks then 0xde shouldn't be appearing.)
Combined with a little command pipelining this leads to 4+GB data stack pools. I'll resume digging tomorrow. Let me know if you need any info; I can reproduce this in seconds.
I couldn't find anything obviously wrong in the code.
I couldn't find anything obviously wrong in the code.
Figured it out. The t_pop in client_handle_input was clobbering imap_clients->command_queue->name. This is because cmd_uid allocated the name from the wrong pool. Here is a patch to fix it. Forget my other patch (to imap_refresh_proctitle). --- a/src/imap/cmd-uid.c (beta5) +++ b/src/imap/cmd-uid.c (working copy) @@ -20,7 +20,7 @@ return TRUE; } - cmd->name = t_strconcat("UID ", cmd_name, NULL); + cmd->name = p_strconcat(cmd->pool, "UID ", cmd_name, NULL); cmd->cmd_flags = command->flags; cmd->func = command->func; cmd->uid = TRUE; Incidentally, this line in client_command_input is puzzling. Comparing a pointer to a character? if (cmd->name == '\0') {
participants (2)
-
Mike Abbott
-
Timo Sirainen