BUG: Using pointer that points to a destructed string's content

BUG: Using pointer that points to a destructed string's content

[Tamas Szakaly]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dear notmuch developers,

The following line is from _notmuch_message_add_directory_terms in
lib/message.cc (line 652 in HEAD):

direntry = (*i).c_str ();

'i' is a Xapian::TermIterator, whose operator* returns a std::string by value.
This means that c_str() is called on a temporary, which is destructed after the
full expression (essentially the particular line in this case), so 'direntry'
will point to a destructed std::string's data.
(See https://gcc.gnu.org/onlinedocs/gcc/Temporaries.html)

One possible modification to correct this issue is using strdup:

direntry = strdup((*i).c_str ());

Note:
Despite the fact that it is wrong, it *generally* works, because delete[]-ing
the underlying character array in the destructor of std::string does not really
touch the memory content, and there is only a minor chance that this memory area
will be allocated again (e.g. from another thread). This caused me some headache
though with 'notmuch new' on FreeBSD 11-CURRENT, where jemalloc is configured so
that freed memory will be filled with 0x5a's.

Best regards,
sghctoma

- -- 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBAgAGBQJUnUiQAAoJEE8tbNCQOSmESAsH/ih+EFx0WJEzImBkNe4I4H+0
Wj9u/ymmpgLwWnV0rg0oxnYoX5T6zT2e1jwTD73H7N4A2Xf2Susjbr6csTP2YyQB
aUbZ5/Ajq+COgpoEXTQUbrIPcIbdl0X05/k9f/OdNqZMHVK6j08hw2oqtpsq6v1+
PiuAa7kKrMda5rzLk08z1/qmJ6D7G2Trl6r5LPfytZhPwrphAJ9bWBofIIJLBQ0R
RdeTmGuzc7FBw1a1JqJWkDL1lI91VTD49Wr/VqYXPbfcWbaHhVYSklDshyEYaK/+
skemzV+aIWJiNHpkALdh3t+070caXlv5hwa826Q4kB0FMmkNlShjFqpXLJToEWo=
=hshP
-----END PGP SIGNATURE-----

Re: BUG: Using pointer that points to a destructed string's content

[David Bremner]

Tamas Szakaly <sghctoma@gmail.com> writes:

> The following line is from _notmuch_message_add_directory_terms in
> lib/message.cc (line 652 in HEAD):
>
> direntry = (*i).c_str ();
>
> 'i' is a Xapian::TermIterator, whose operator* returns a std::string by value.
> This means that c_str() is called on a temporary, which is destructed after the
> full expression (essentially the particular line in this case), so 'direntry'
> will point to a destructed std::string's data.
> (See https://gcc.gnu.org/onlinedocs/gcc/Temporaries.html)

Does the following patch fix it for you? I have to double check that
direntry wasn't needed for something, but the test suite passes ;).

diff --git a/lib/message.cc b/lib/message.cc
index a7a13cc..24d0d5b 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -649,10 +649,8 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
 	/* Indicate that there are filenames remaining. */
 	status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
 
-	direntry = (*i).c_str ();
-	direntry += direntry_prefix_len;
-
-	directory_id = strtol (direntry, &colon, 10);
+	directory_id = strtol (
+	    (*i).c_str () + direntry_prefix_len, &colon, 10);
 
 	if (colon == NULL || *colon != ':')
 	    INTERNAL_ERROR ("malformed direntry");

Re: BUG: Using pointer that points to a destructed string's content

[Tamas Szakaly]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> > The following line is from _notmuch_message_add_directory_terms in
> > lib/message.cc (line 652 in HEAD):
> >
> > direntry = (*i).c_str ();
> >
> > 'i' is a Xapian::TermIterator, whose operator* returns a std::string by value.
> > This means that c_str() is called on a temporary, which is destructed after the
> > full expression (essentially the particular line in this case), so 'direntry'
> > will point to a destructed std::string's data.
> > (See https://gcc.gnu.org/onlinedocs/gcc/Temporaries.html)
> 
> Does the following patch fix it for you? I have to double check that
> direntry wasn't needed for something, but the test suite passes ;).
> 
> diff --git a/lib/message.cc b/lib/message.cc
> index a7a13cc..24d0d5b 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -649,10 +649,8 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
>  	/* Indicate that there are filenames remaining. */
>  	status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
>  
> -	direntry = (*i).c_str ();
> -	direntry += direntry_prefix_len;
> -
> -	directory_id = strtol (direntry, &colon, 10);
> +	directory_id = strtol (
> +	    (*i).c_str () + direntry_prefix_len, &colon, 10);
>  
>  	if (colon == NULL || *colon != ':')
>  	    INTERNAL_ERROR ("malformed direntry");
> 

Nope, it's still not correct: with this code "colon" will point into the
temporary string's data. I'm using this patch now:

- --- work/notmuch-0.18.1/lib/message.cc	2014-06-25 12:30:10.000000000 +0200
+++ /root/message.cc	2014-12-20 08:15:10.000000000 +0100
@@ -601,18 +601,19 @@
 	unsigned int directory_id;
 	const char *direntry, *directory;
 	char *colon;
+	const std::string& tmp = *i;
 
+	direntry = tmp.c_str ();
+	
 	/* Terminate loop at first term without desired prefix. */
- -	if (strncmp ((*i).c_str (), direntry_prefix, direntry_prefix_len))
+	if (strncmp (direntry, direntry_prefix, direntry_prefix_len))
 	    break;
 
 	/* Indicate that there are filenames remaining. */
 	status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
 
- -	direntry = (*i).c_str ();
- -	direntry += direntry_prefix_len;
- -
- -	directory_id = strtol (direntry, &colon, 10);
+	directory_id = strtol (
+		direntry + direntry_prefix_len, &colon, 10);
 
 	if (colon == NULL || *colon != ':')
 	    INTERNAL_ERROR ("malformed direntry");

This way operator* and c_str will be called only once, and its also better than
my first suggestion (using strdup), since it requires no additional memory
allocation. 

Regards,
sghctoma
- -- 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBAgAGBQJUneoOAAoJEE8tbNCQOSmEzTwH/2NtrBberwi1nPdNgOLm2j9g
StrfYLnoIJIjn3dU7/X22QnX7CQHhXDMa+bY7UHDiS+oNTmMfRg0j6t1DV/tA/Pv
gA2Ti80zP8B1c0YX/KELGQYcxSQTE/p4hFe0Ff/Yo1Qi4KvFntxBiuvB6I1quQmX
1Kxew2l/Oq2PLOfqtmqfg5GuoDMXiNjNQgmfsV6x+i9F5PR3qnuvrzZvUWCC0URX
Xx2w1P+JYBambLdMqOH9MmU4ck/lobCkxprcfUK27i/LSNsl2UH+650bjef3Y6gR
CoErHJ4xso9+T8Dk+zRTtK1+uAs66xrLYRverJAnIL3LXFQG+czhUzuAdTaFQCM=
=PqvU
-----END PGP SIGNATURE-----

[PATCH] lib: collapse computation of directory_id into a single expression

[David Bremner]

According to id:20141226113755.GA64154@pamparam , the lifetime
of (*i).c_str is only guaranteed for that line, so we extract the
information we need in the same line.
---
 lib/message.cc | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index a7a13cc..14e60af 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -639,7 +639,7 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
 
     for (i.skip_to (direntry_prefix); i != message->doc.termlist_end (); i++) {
 	unsigned int directory_id;
-	const char *direntry, *directory;
+	const char *directory;
 	char *colon;
 
 	/* Terminate loop at first term without desired prefix. */
@@ -649,10 +649,9 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
 	/* Indicate that there are filenames remaining. */
 	status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
 
-	direntry = (*i).c_str ();
-	direntry += direntry_prefix_len;
-
-	directory_id = strtol (direntry, &colon, 10);
+	directory_id =
+	    strtol ((*i).c_str () + direntry_prefix_len,
+		    &colon, 10);
 
 	if (colon == NULL || *colon != ':')
 	    INTERNAL_ERROR ("malformed direntry");
-- 
2.1.3

[PATCH] lib: another iterator-temporary/stale-pointer bug

[David Bremner]

Tamas Szakaly points out [1] that the bug fixed in 51b073c still
exists in at least one place. This change follows the suggestion of
[2] and creates a block scope temporary std::string to avoid the rules
of iterators temporaries.

[1]: id:20141226113755.GA64154@pamparam
[2]: id:20141226230655.GA41992@pamparam
---

I decided to take a more minimalist approach than [2]. In particular
using "direntry" for two different things seemed slightly trickier
than necessary, for no obvious performance gain (calling .c_str()
should be cheap).

lib/message.cc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index a7a13cc..bacb4d4 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -641,15 +641,16 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
 	unsigned int directory_id;
 	const char *direntry, *directory;
 	char *colon;
+	const std::string term = *i;
 
 	/* Terminate loop at first term without desired prefix. */
-	if (strncmp ((*i).c_str (), direntry_prefix, direntry_prefix_len))
+	if (strncmp (term.c_str (), direntry_prefix, direntry_prefix_len))
 	    break;
 
 	/* Indicate that there are filenames remaining. */
 	status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
 
-	direntry = (*i).c_str ();
+	direntry = term.c_str ();
 	direntry += direntry_prefix_len;
 
 	directory_id = strtol (direntry, &colon, 10);
-- 
2.1.3

Re: [PATCH] lib: another iterator-temporary/stale-pointer bug

[Jani Nikula]

On Sun, 28 Dec 2014, David Bremner <david@tethera.net> wrote:
> Tamas Szakaly points out [1] that the bug fixed in 51b073c still
> exists in at least one place. This change follows the suggestion of
> [2] and creates a block scope temporary std::string to avoid the rules
> of iterators temporaries.
>
> [1]: id:20141226113755.GA64154@pamparam
> [2]: id:20141226230655.GA41992@pamparam
> ---
>
> I decided to take a more minimalist approach than [2]. In particular
> using "direntry" for two different things seemed slightly trickier
> than necessary, for no obvious performance gain (calling .c_str()
> should be cheap).
>
> lib/message.cc | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/message.cc b/lib/message.cc
> index a7a13cc..bacb4d4 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -641,15 +641,16 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
>  	unsigned int directory_id;
>  	const char *direntry, *directory;
>  	char *colon;
> +	const std::string term = *i;

You could use a reference here like in [2]. Either way, LGTM.

Jani.

>  
>  	/* Terminate loop at first term without desired prefix. */
> -	if (strncmp ((*i).c_str (), direntry_prefix, direntry_prefix_len))
> +	if (strncmp (term.c_str (), direntry_prefix, direntry_prefix_len))
>  	    break;
>  
>  	/* Indicate that there are filenames remaining. */
>  	status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
>  
> -	direntry = (*i).c_str ();
> +	direntry = term.c_str ();
>  	direntry += direntry_prefix_len;
>  
>  	directory_id = strtol (direntry, &colon, 10);
> -- 
> 2.1.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] lib: convert two "iterator copy strings" into references.

[David Bremner]

Apparently this is a supported and even idiomatic way of keeping a
temporary object (e.g. like that returned from an operator
dereference) alive.
---

I decided it was better to do both of these "keepalive" strings the
same way, so I applied the parent patch as is.

 lib/message.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index bacb4d4..956a70a 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -283,7 +283,7 @@ _notmuch_message_get_term (notmuch_message_t *message,
     if (i == end)
 	return NULL;
 
-    std::string term = *i;
+    const std::string &term = *i;
     if (strncmp (term.c_str(), prefix, prefix_len))
 	return NULL;
 
@@ -641,7 +641,7 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
 	unsigned int directory_id;
 	const char *direntry, *directory;
 	char *colon;
-	const std::string term = *i;
+	const std::string &term = *i;
 
 	/* Terminate loop at first term without desired prefix. */
 	if (strncmp (term.c_str (), direntry_prefix, direntry_prefix_len))
-- 
2.1.3

Re: [PATCH] lib: convert two "iterator copy strings" into references.

[Jani Nikula]

On Fri, 02 Jan 2015, David Bremner <david@tethera.net> wrote:
> Apparently this is a supported and even idiomatic way of keeping a
> temporary object (e.g. like that returned from an operator
> dereference) alive.
> ---
>
> I decided it was better to do both of these "keepalive" strings the
> same way, so I applied the parent patch as is.

LGTM.

>
>  lib/message.cc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/message.cc b/lib/message.cc
> index bacb4d4..956a70a 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -283,7 +283,7 @@ _notmuch_message_get_term (notmuch_message_t *message,
>      if (i == end)
>  	return NULL;
>  
> -    std::string term = *i;
> +    const std::string &term = *i;
>      if (strncmp (term.c_str(), prefix, prefix_len))
>  	return NULL;
>  
> @@ -641,7 +641,7 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
>  	unsigned int directory_id;
>  	const char *direntry, *directory;
>  	char *colon;
> -	const std::string term = *i;
> +	const std::string &term = *i;
>  
>  	/* Terminate loop at first term without desired prefix. */
>  	if (strncmp (term.c_str (), direntry_prefix, direntry_prefix_len))
> -- 
> 2.1.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] lib: convert two "iterator copy strings" into references.

[Tomi Ollila]

On Fri, Jan 02 2015, David Bremner <david@tethera.net> wrote:

> Apparently this is a supported and even idiomatic way of keeping a
> temporary object (e.g. like that returned from an operator
> dereference) alive.
> ---

Based on internet search with words 'c++ reference to temporary' (*)
I believe the change is working one.

Tomi

(*) Uh, reading those explations gives me a headache >;)

>
> I decided it was better to do both of these "keepalive" strings the
> same way, so I applied the parent patch as is.
>
>  lib/message.cc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/message.cc b/lib/message.cc
> index bacb4d4..956a70a 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -283,7 +283,7 @@ _notmuch_message_get_term (notmuch_message_t *message,
>      if (i == end)
>  	return NULL;
>  
> -    std::string term = *i;
> +    const std::string &term = *i;
>      if (strncmp (term.c_str(), prefix, prefix_len))
>  	return NULL;
>  
> @@ -641,7 +641,7 @@ _notmuch_message_add_directory_terms (void *ctx, notmuch_message_t *message)
>  	unsigned int directory_id;
>  	const char *direntry, *directory;
>  	char *colon;
> -	const std::string term = *i;
> +	const std::string &term = *i;
>  
>  	/* Terminate loop at first term without desired prefix. */
>  	if (strncmp (term.c_str (), direntry_prefix, direntry_prefix_len))
> -- 
> 2.1.3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] lib: convert two "iterator copy strings" into references.

[David Bremner]

David Bremner <david@tethera.net> writes:

> Apparently this is a supported and even idiomatic way of keeping a
> temporary object (e.g. like that returned from an operator
> dereference) alive.

pushed.

d

Re: BUG: Using pointer that points to a destructed string's content

[David Bremner]

Tamas Szakaly <sghctoma@gmail.com> writes:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Dear notmuch developers,
>
> The following line is from _notmuch_message_add_directory_terms in
> lib/message.cc (line 652 in HEAD):
>
> direntry = (*i).c_str ();
>

This should be fixed in commit 3d978a0d

d