Sorry, sent the wrong version, please see amended one attached.

Aki

> On October 17, 2016 at 10:18 PM Aki Tuomi <aki.tu...@dovecot.fi> wrote:
> 
> 
> Oh duh, it used datastack pool. Try again with the attached patch? Please 
> remember to clear the previous one out before trying.
> 
> Aki
> 
> > 
> > The trace is missing some symbols, I will debug tomorrow and see where
> > the call comes from exactly.
> > 
> > Regards,
> > 
> > 
> > On 10/17/2016 06:23 PM, Aki Tuomi wrote:
> > > Hi!
> > > 
> > > Looking at the code, I think the bug is that it just copies pointer to 
> > > the value, it should, instead, duplicate the value to some memory region. 
> > > Can you see if this following patch fixes it?
> > > 
> > > Aki
> > > 
> > >> On October 17, 2016 at 4:14 PM Pierre Jaury <pie...@jaury.eu> wrote:
> > >>
> > >>
> > >> Okay, it seems to me that the bug is due to "t_str_tabunescape" using
> > >> the unsafe datastack ("t_strdup_noconst") while the string is actually
> > >> returned in an async callback.
> > >>
> > >> Before it is handled by "client_dict_lookup", "client_dict_wait"
> > >> actually fires some IO loops that are more than likely to call "t_pop"
> > >> and free/flush the result string (I checked, it does call "t_pop" a
> > >> couple times indeed). Maybe it would be safer to use a standard
> > >> datastack pool when unescaping a string in that context.
> > >>
> > >> I could work on a patch that would:
> > >>
> > >>  - add an optional "pool" attribute to the "client_dict_cmd" structure,
> > >>  - pass the pool to the async lookup function,
> > >>  - use the pool when escaping strings that should survive the callback
> > >>    chain
> > >>
> > >> What do you think?
> > >>
> > >> Regards,
> > >> kaiyou
> > >>
> > >>
> > >> On 10/17/2016 09:52 AM, Pierre Jaury wrote:
> > >>> While trying to isolate properly and reproduce, I was able to trigger
> > >>> the same bug with the following code:
> > >>>
> > >>>   struct dict *dict;
> > >>>   char* dict_uri = "proxy::sieve";
> > >>>   char* key = "priv/key";
> > >>>   char* username = "ad...@domain.tld";
> > >>>   char* value, error;
> > >>>
> > >>>   dict_drivers_register_builtin();
> > >>>   dict_init(dict_uri, DICT_DATA_TYPE_STRING, username,
> > >>>     doveadm_settings->base_dir, &dict, &error);
> > >>>   dict_lookup(dict, pool_datastack_create(), key, &value);
> > >>>   printf(">%s\n", value); // outputs an empty string
> > >>>   dict_deinit(&dict);
> > >>>
> > >>> I trimmed it to the bare minimal string manipulation functions involved
> > >>> but cannot reproduce in that case:
> > >>>
> > >>>   pool_t pool = pool_datastack_create();
> > >>>
> > >>>   char* s1 = "test\001n\001rtest";
> > >>>   char* s2 = t_str_tabunescape(s1);
> > >>>   char* s3 = p_strdup(pool, s2);
> > >>>
> > >>>   printf("1>%s\n", s1);
> > >>>   printf("2>%s\n", s2);
> > >>>   printf("3>%s\n", s3); // all three output the string with NL and CR
> > >>>
> > >>> Maybe I am missing a function call in the process or maybe the issue is
> > >>> related to the way unescaping is performed in the async callback
> > >>> function in "dict-client.c", or maybe even some other edge case.
> > >>>
> > >>> Finally, I was able to run the first snippet without bug by removing the
> > >>> string duplication in "t_str_tabunescape" (which I realize is not a
> > >>> proper solution), or by explicitely using the following pool:
> > >>>
> > >>>   return str_tabunescape(p_strdup(pool_datastack_create(), str));
> > >>>
> > >>>
> > >>> Hope this helps.
> > >>>
> > >>> kaiyou
> > >>>
> > >>>
> > >>> On 10/17/2016 07:51 AM, Aki Tuomi wrote:
> > >>>> Hi!
> > >>>>
> > >>>> This does sound like a bug, we'll have look.
> > >>>>
> > >>>> Aki
> > >>>>
> > >>>>
> > >>>> On 17.10.2016 01:26, Pierre Jaury wrote:
> > >>>>> I dived a little bit further into the rabbit hole, up to the point 
> > >>>>> where
> > >>>>> debugging has become unpracticle but I still haven't found the root
> > >>>>> cause for sure.
> > >>>>>
> > >>>>> I read most of the code for "p_strdup" based on datastack memory pools
> > >>>>> (which are used for dictionary lookups both with doveadm and by 
> > >>>>> extdata)
> > >>>>> and it seems ok. Still, after "t_malloc_real" is called in 
> > >>>>> "t_malloc0",
> > >>>>> the allocated buffer has the same address as the source string.
> > >>>>>
> > >>>>> The only sensible explanation I can come up with is that during
> > >>>>> unescaping, strings are not allocated properly, leading to the memory
> > >>>>> pool reusing the string address and zeroing it in the process before 
> > >>>>> the
> > >>>>> string copy operation.
> > >>>>>
> > >>>>> I will follow on this path tomorrow, any lead is more than welcome.
> > >>>>>
> > >>>>> kaiyou.
> > >>>>>
> > >>>>> On 10/16/2016 11:16 PM, Pierre Jaury wrote:
> > >>>>>> Hello,
> > >>>>>>
> > >>>>>> I am using a dict proxy for my sieve extdata plugin to access some
> > >>>>>> fields from an SQLite database (autoreply text and other
> > >>>>>> database-configured items).
> > >>>>>>
> > >>>>>> All tests are performed against version 2.2.25.
> > >>>>>>
> > >>>>>>   $ dovecot --version
> > >>>>>>   2.2.25 (7be1766)
> > >>>>>>
> > >>>>>> My configuration looks like:
> > >>>>>>
> > >>>>>>   dict {
> > >>>>>>     sieve = sqlite:/etc/dovecot/pigeonhole-sieve.dict
> > >>>>>>   }
> > >>>>>>
> > >>>>>>   [...]
> > >>>>>>   sieve_extdata_dict_uri = proxy::sieve
> > >>>>>>
> > >>>>>> I am able to read pretty much any attribute without any issue, except
> > >>>>>> when the value contains a special character like "\r" or "\n". By 
> > >>>>>> using
> > >>>>>> the doveadm dict client, I narrowed it down to the dictionary 
> > >>>>>> management
> > >>>>>> part (either server, protocol or client).
> > >>>>>>
> > >>>>>> I was suspecting escaping functions from "lib/strescape.c" (mostly
> > >>>>>> str_tabescape and its counterpart, used by "lib-dict/client.c"), so I
> > >>>>>> monitored socket communications. It seems that escaping is done 
> > >>>>>> properly
> > >>>>>> on the server and the socket is not an issue either.
> > >>>>>>
> > >>>>>> The following strace dump results from running "doveadm dict get"
> > >>>>>> against the dict socket:
> > >>>>>>
> > >>>>>>   connect(8, {sa_family=AF_UNIX, sun_path="..."}, 110) = 0
> > >>>>>>   fstat(8, {st_mode=S_IFSOCK|0777, st_size=0, ...}) = 0
> > >>>>>>   [...]
> > >>>>>>   write(8, "H2\t0\t0\tad...@domain.tld\tsieve\n", 30) = 30
> > >>>>>>   [...]
> > >>>>>>   read(8, "Otest\1r\1ntest\n", 8192)      = 14
> > >>>>>>
> > >>>>>> Indeed "\1r" and "\1n" are the escape sequences used by
> > >>>>>> "lib/strescape.c". I went deeped and debugged the call to 
> > >>>>>> "dict_lookup"
> > >>>>>> performed by doveadm. Indeed the client gets the proper string from 
> > >>>>>> the
> > >>>>>> socket and to my surprise, it is properly unescaped.
> > >>>>>>
> > >>>>>> Then, in "client_dict_lookup" ("lib-dict/dict-client.c"), the call to
> > >>>>>> "p_strdup" returns an empty string (null byte set at the target 
> > >>>>>> address).
> > >>>>>>
> > >>>>>> Before the call to the dict "->lookup" attribute 
> > >>>>>> (client_dict_lookup):
> > >>>>>>
> > >>>>>>    RAX: 0x7ffff73a37c0 (push   r14)
> > >>>>>>    RBX: 0x6831b8 ("priv/reply_body")
> > >>>>>>    RCX: 0x7fffffffe240 --> 0x682a60 --> 0x6831b8 ("priv/reply_body")
> > >>>>>>    RDX: 0x6831b8 ("priv/reply_body")
> > >>>>>>    RSI: 0x683288 --> 0x7ffff7653120 --> 0x7ffff73ea620 ([...])
> > >>>>>>    RDI: 0x690ad0 --> 0x7ffff7400713 --> 0x75250079786f7270 ('proxy')
> > >>>>>>
> > >>>>>>    0x7ffff73a1f10 <dict_lookup+32>:  mov    rcx,r11 (value_r)
> > >>>>>>    0x7ffff73a1f13 <dict_lookup+35>:  mov    rdx,r8 (key)
> > >>>>>>    0x7ffff73a1f16 <dict_lookup+38>:  mov    rsi,r10 (pool)
> > >>>>>>    0x7ffff73a1f19 <dict_lookup+41>:  mov    rdi,r9 (dict)
> > >>>>>>    0x7ffff73a1f1c <dict_lookup+44>:  add    rsp,0x8
> > >>>>>> => 0x7ffff73a1f20 <dict_lookup+48>:  jmp    rax
> > >>>>>>
> > >>>>>> Before the call to p_strdup in "client_dict_lookup":
> > >>>>>>
> > >>>>>>    RSI: 0x6832d8 ("test\r\ntest") (lookup.result.value)
> > >>>>>>    RDI: 0x683288 --> 0x7ffff7653120 --> [...] (pool)
> > >>>>>>    RAX: 0x0 (result)
> > >>>>>>
> > >>>>>>    0x7ffff73a384f:   nop
> > >>>>>>    0x7ffff73a3850:   mov    rsi,QWORD PTR [rsp+0x8]
> > >>>>>>    0x7ffff73a3855:   mov    rdi,r14
> > >>>>>> => 0x7ffff73a3858:   call   0x7ffff736d3c0 <p_strdup@plt>
> > >>>>>>    0x7ffff73a385d:   mov    QWORD PTR [r13+0x0],rax
> > >>>>>>    0x7ffff73a3861:   mov    rsi,QWORD PTR [rsp+0x18]
> > >>>>>>    0x7ffff73a3866:   xor    rsi,QWORD PTR fs:0x28
> > >>>>>>    0x7ffff73a386f:   mov    eax,ebx
> > >>>>>>
> > >>>>>> After the call:
> > >>>>>>
> > >>>>>>    0x7ffff73a3850:   mov    rsi,QWORD PTR [rsp+0x8]
> > >>>>>>    0x7ffff73a3855:   mov    rdi,r14
> > >>>>>>    0x7ffff73a3858:   call   0x7ffff736d3c0 <p_strdup@plt>
> > >>>>>> => 0x7ffff73a385d:   mov    QWORD PTR [r13+0x0],rax
> > >>>>>>    0x7ffff73a3861:   mov    rsi,QWORD PTR [rsp+0x18]
> > >>>>>>    0x7ffff73a3866:   xor    rsi,QWORD PTR fs:0x28
> > >>>>>>    0x7ffff73a386f:   mov    eax,ebx
> > >>>>>>    0x7ffff73a3871:   jne    0x7ffff73a38da
> > >>>>>>
> > >>>>>>    RSI: 0x0
> > >>>>>>    RDI: 0x6832d8 --> 0x0
> > >>>>>>    RAX: 0x6832d8 --> 0x0 (result)
> > >>>>>>
> > >>>>>> It is worth noting that I can reproduce the exact same execution flow
> > >>>>>> with a non-multiline result string (lookup.result.value) that is
> > >>>>>> properly copied by "p_strdup" and returned in RAX, then displayed by
> > >>>>>> doveadm.
> > >>>>>>
> > >>>>>> I am not familiar with the pooling mechanism hidden behind the call 
> > >>>>>> to
> > >>>>>> p_strdump and not quite sure why this behaviour is emerging. Maybe I 
> > >>>>>> am
> > >>>>>> even miles away from an understanding of the issue here, but it 
> > >>>>>> sounds
> > >>>>>> to me like something is wrong in the way "p_strdup" performs the 
> > >>>>>> copy.
> > >>>>>>
> > >>>>>> Hope this helps,
> > >>>>>> kaiyou.
> > >>>>>>
> > >>>>>>
> > >>>>> >
From f18239055b2a51c6a69c4bdbaf45edd63546ccc5 Mon Sep 17 00:00:00 2001
From: Aki Tuomi <aki.tu...@dovecot.fi>
Date: Mon, 17 Oct 2016 19:21:34 +0300
Subject: [PATCH] lib-dict: Duplicate result value in mempool

Duplicate the result value already in callback
to avoid it getting lost during async calls.
---
 src/lib-dict/dict-client.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/lib-dict/dict-client.c b/src/lib-dict/dict-client.c
index 5388550..060c5ca 100644
--- a/src/lib-dict/dict-client.c
+++ b/src/lib-dict/dict-client.c
@@ -817,6 +817,7 @@ client_dict_lookup_async(struct dict *_dict, const char *key,
 }
 
 struct client_dict_sync_lookup {
+	pool_t pool;
 	struct dict_lookup_result result;
 	char *error;
 };
@@ -826,18 +827,22 @@ static void client_dict_lookup_callback(const struct dict_lookup_result *result,
 {
 	struct client_dict_sync_lookup *lookup = context;
 
-	lookup->result = *result;
+	lookup->result.ret = result->ret;
 	if (result->ret == -1)
 		lookup->error = i_strdup(result->error);
+	else if (result->ret == 1)
+		lookup->result.value = i_strdup(result->value);
 }
 
 static int client_dict_lookup(struct dict *_dict, pool_t pool, const char *key,
 			      const char **value_r, const char **error_r)
 {
 	struct client_dict_sync_lookup lookup;
+	char *value;
 
 	memset(&lookup, 0, sizeof(lookup));
 	lookup.result.ret = -2;
+	lookup.pool = pool;
 
 	client_dict_lookup_async(_dict, key, client_dict_lookup_callback, &lookup);
 	if (lookup.result.ret == -2)
@@ -853,6 +858,8 @@ static int client_dict_lookup(struct dict *_dict, pool_t pool, const char *key,
 		return 0;
 	case 1:
 		*value_r = p_strdup(pool, lookup.result.value);
+		value = (char*)lookup.result.value;
+		i_free(value);
 		return 1;
 	}
 	i_unreached();
-- 
2.7.4

Reply via email to