Date: Sat, 27 Dec 2014 00:06:55 +0100
From: Tamas Szakaly <sghctoma@gmail.com>
To: notmuch@notmuchmail.org
Subject: Re: BUG: Using pointer that points to a destructed string's content
Message-ID: <20141226230655.GA41992@pamparam>
Mail-Followup-To: notmuch@notmuchmail.org
References: <20141226113755.GA64154@pamparam>
	<87oaqqf4ri.fsf@maritornes.cs.unb.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; x-action=pgp-signed
In-Reply-To: <87oaqqf4ri.fsf@maritornes.cs.unb.ca>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: list

-----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-----