[PATCH] don't store temporary value returned from c_str()

[PATCH] don't store temporary value returned from c_str()

[Vladimir.Marek]

From: Vladimir Marek <vlmarek@volny.cz>

This is causing problems when compiled by Oracle Studio. Memory pointed
by (const char*)term was already changed once talloc_strdup was called.

Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
---
 lib/message.cc |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 8720c1b..8d329d1 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -266,18 +266,17 @@ _notmuch_message_get_term (notmuch_message_t *message,
 			   const char *prefix)
 {
     int prefix_len = strlen (prefix);
-    const char *term = NULL;
     char *value;
 
     i.skip_to (prefix);
 
-    if (i != end)
-	term = (*i).c_str ();
+    if (i == end)
+	return NULL;
 
-    if (!term || strncmp (term, prefix, prefix_len))
+    if (strncmp ((*i).c_str(), prefix, prefix_len))
 	return NULL;
 
-    value = talloc_strdup (message, term + prefix_len);
+    value = talloc_strdup (message, (*i).c_str() + prefix_len);
 
 #if DEBUG_DATABASE_SANITY
     i++;
-- 
1.7.3.2

Re: [PATCH] don't store temporary value returned from c_str()

[Tomi Ollila]

On Sat, Apr 20 2013, Vladimir.Marek@oracle.com wrote:

> From: Vladimir Marek <vlmarek@volny.cz>
>
> This is causing problems when compiled by Oracle Studio. Memory pointed
> by (const char*)term was already changed once talloc_strdup was called.

If that changes, I'd like to understand why (and stated in the commit
message). If that is clear to everyone else I will withdraw the question --
I am not too familiar with these iterators magic... :D

Tomi

>
> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
> ---
>  lib/message.cc |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/lib/message.cc b/lib/message.cc
> index 8720c1b..8d329d1 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -266,18 +266,17 @@ _notmuch_message_get_term (notmuch_message_t *message,
>  			   const char *prefix)
>  {
>      int prefix_len = strlen (prefix);
> -    const char *term = NULL;
>      char *value;
>  
>      i.skip_to (prefix);
>  
> -    if (i != end)
> -	term = (*i).c_str ();
> +    if (i == end)
> +	return NULL;
>  
> -    if (!term || strncmp (term, prefix, prefix_len))
> +    if (strncmp ((*i).c_str(), prefix, prefix_len))
>  	return NULL;
>  
> -    value = talloc_strdup (message, term + prefix_len);
> +    value = talloc_strdup (message, (*i).c_str() + prefix_len);
>  
>  #if DEBUG_DATABASE_SANITY
>      i++;
> -- 
> 1.7.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] don't store temporary value returned from c_str()

[Vladimir Marek]

> > From: Vladimir Marek <vlmarek@volny.cz>
> >
> > This is causing problems when compiled by Oracle Studio. Memory pointed
> > by (const char*)term was already changed once talloc_strdup was called.
> 
> If that changes, I'd like to understand why (and stated in the commit
> message). If that is clear to everyone else I will withdraw the question --
> I am not too familiar with these iterators magic... :D

Well, a) standards says that

A temporary bound to a reference parameter in a function call (5.2.2)
persists until the completion of the full expression containing the call

(you can find the message all over the net, but I can't find actual link
to the standard :-/)


b) Imagine the function c_str() looks like that:

const char*
string::c_str(void) {
	char buf[100];

	strcpy (buf, this->internal_representation);
	return buf;
}

you can't do:
char *my_tmp = string.c_str();
printf(my_tmp);

you can do:
printf(string.c_str());

Hopefully I made the example right ...

Cheers
-- 
	Vlad

Re: [PATCH] don't store temporary value returned from c_str()

[David Bremner]

Vladimir Marek <Vladimir.Marek@Oracle.COM> writes:

>
> const char*
> string::c_str(void) {
> 	char buf[100];
>
> 	strcpy (buf, this->internal_representation);
> 	return buf;
> }

Isn't that undefined behavior, returning a pointer to a (non-static)
local variable?

d

Re: [PATCH] don't store temporary value returned from c_str()

[Vladimir Marek]

> > const char*
> > string::c_str(void) {
> > 	char buf[100];
> >
> > 	strcpy (buf, this->internal_representation);
> > 	return buf;
> > }
> 
> Isn't that undefined behavior, returning a pointer to a (non-static)
> local variable?

Right, I was trying to bring up an example and this one is not very
good. Maybe something like that:


const char*
c_str() {
	vector<char> X(my_string);
	return &X[0];
};

char buf[];
strcpy(buf, c_str());

X destructor is called after strcpy is done.

char buf[];
char *tmp=c_str();
strcpy(buf, tmp);

X destructor is called before strcpy.

At least this is how I understand it.

-- 
	Vlad

Re: [PATCH] don't store temporary value returned from c_str()

[Kim Minh Kaplan]

Vladimir Marek writes:

> Well, a) standards says that
>
> A temporary bound to a reference parameter in a function call (5.2.2)
> persists until the completion of the full expression containing the call
>
> (you can find the message all over the net, but I can't find actual link
> to the standard :-/)

The january 2012 working draft: 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

    12.2 Temporary objects [class.temporary]

    1 Temporaries of class type are created in various contexts: binding a
    reference to a prvalue (8.5.3), returning a prvalue (6.6.3) […]

    3 When an implementation introduces a temporary object of a class that
    has a non-trivial constructor (12.1, 12.8), it shall ensure that a
    constructor is called for the temporary object. Similarly, the
    destructor shall be called for a temporary with a non-trivial destructor
    (12.4). Temporary objects are destroyed as the last step in evaluating
    the full-expression (1.9) that (lexically) contains the point where they
    were created.

Kim Minh.

Re: [PATCH] don't store temporary value returned from c_str()

[Vladimir Marek]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

Hi,

Thank you, I found it eventually too. But I wrote little test program
(attached) which confused me. I haven't had much time to take a look
into it since weekend.

The idea is to have temporary object where I can detect whether
destructor was called.

I thought that

printf ("%s\n", s.c_str());
will print "test"

and

x=s.c_str();
printf ("%s\n", x);

will print "destroyed"

On my machine both prints "destroyed".


I still believe my fix is correct, but I'm not at the position to be
able to defend it at the moment :)

Thank you
-- 
	Vlad

> The january 2012 working draft: 
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
> 
>     12.2 Temporary objects [class.temporary]
> 
>     1 Temporaries of class type are created in various contexts: binding a
>     reference to a prvalue (8.5.3), returning a prvalue (6.6.3) […]
> 
>     3 When an implementation introduces a temporary object of a class that
>     has a non-trivial constructor (12.1, 12.8), it shall ensure that a
>     constructor is called for the temporary object. Similarly, the
>     destructor shall be called for a temporary with a non-trivial destructor
>     (12.4). Temporary objects are destroyed as the last step in evaluating
>     the full-expression (1.9) that (lexically) contains the point where they
>     were created.

[-- Attachment #2: a.cc --]
[-- Type: text/plain, Size: 603 bytes --]

#include <stdio.h>
#include <vector>
#include <string.h>

using namespace std;

class array {
	vector<char> impl;

	public:

	array(int size):impl(size) { }
	array(array &in):impl(in.impl) { }

	char*
	operator[](size_t i) {
		return &impl[i];
	};

	~array() {
		strcpy(&impl[0], "destroyed");
	}
};

class str {
	array tmp;

	public:

	str(const char *input):tmp(100) {
		strcpy(tmp[0], input);
	};

	const char* c_str() {
		return (array(tmp))[0];
	};
};


int
main (int argc, char **argv)
{
	str s("test");
	const char *x;
	printf ("%s\n", s.c_str());
	x=s.c_str();
	printf ("%s\n", x);
	return 0;
}

Re: [PATCH] don't store temporary value returned from c_str()

[Kim Minh Kaplan]

Vladimir Marek :

> Thank you, I found it eventually too. But I wrote little test program
> (attached) which confused me. I haven't had much time to take a look
> into it since weekend.
>
> The idea is to have temporary object where I can detect whether
> destructor was called.
>
> I thought that
>
> printf ("%s\n", s.c_str());
> will print "test"
>
> and
>
> x=s.c_str();
> printf ("%s\n", x);
>
> will print "destroyed"
>
> On my machine both prints "destroyed".

You have to somehow instantiate a temporary object, using a function
call for example.

    #include <string.h>
    #include <stdio.h>
    struct example {
      char data[10];
      char *c_str() { return data; }
     example() { strcpy(data, "test"); }
      ~example() { strcpy(data, "destroyed"); }
    };
    example foo()
    {
      example res;
      return res;
    }
    main()
    {
      printf("%s\n", foo().c_str());
      char *x = foo().c_str();
      printf("%s\n", x);
    }

Kim Minh.

Re: [PATCH] don't store temporary value returned from c_str()

[Vladimir Marek]

> You have to somehow instantiate a temporary object, using a function
> call for example.
> 
>     #include <string.h>
>     #include <stdio.h>
>     struct example {
>       char data[10];
>       char *c_str() { return data; }
>      example() { strcpy(data, "test"); }
>       ~example() { strcpy(data, "destroyed"); }
>     };
>     example foo()
>     {
>       example res;
>       return res;
>     }
>     main()
>     {
>       printf("%s\n", foo().c_str());
>       char *x = foo().c_str();
>       printf("%s\n", x);
>     }

Right. After more tests I think I'm getting hang of it. In my examples I
had function similar to

char *foo() {
  example res;
  return res.c_str();
}

In this case the reference returned is to _char pointer_ and not to
example struct. So the struct is destroyed before returning from foo to
caller. Compiler does not understand that the returned char pointer does
not have any meaning without the example struct.

This is where I was doing the mistake, I believe.

Thank you
-- 
	Vlad

Re: [PATCH] don't store temporary value returned from c_str()

[Jani Nikula]

On Sat, 20 Apr 2013, Vladimir.Marek@oracle.com wrote:
> From: Vladimir Marek <vlmarek@volny.cz>
>
> This is causing problems when compiled by Oracle Studio. Memory pointed
> by (const char*)term was already changed once talloc_strdup was called.
>
> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
> ---
>  lib/message.cc |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/lib/message.cc b/lib/message.cc
> index 8720c1b..8d329d1 100644
> --- a/lib/message.cc
> +++ b/lib/message.cc
> @@ -266,18 +266,17 @@ _notmuch_message_get_term (notmuch_message_t *message,
>  			   const char *prefix)
>  {
>      int prefix_len = strlen (prefix);
> -    const char *term = NULL;
>      char *value;
>  
>      i.skip_to (prefix);
>  
> -    if (i != end)
> -	term = (*i).c_str ();

It's okay to use the result of .c_str() as long as the string object
stays in scope, and none of the non-const member functions are
called. Here, I think the problem is that TermIterator's overloaded
operator*() returns a string object within the if block's scope, and it
goes immediately out of scope. You could check this by adding

	string s = *i;

in function scope, and replacing (*i) with s in the if block. This might
also be more obvious than the presented patch, but I think the patch is
fine too.

BR,
Jani.


> +    if (i == end)
> +	return NULL;
>  
> -    if (!term || strncmp (term, prefix, prefix_len))
> +    if (strncmp ((*i).c_str(), prefix, prefix_len))
>  	return NULL;
>  
> -    value = talloc_strdup (message, term + prefix_len);
> +    value = talloc_strdup (message, (*i).c_str() + prefix_len);
>  
>  #if DEBUG_DATABASE_SANITY
>      i++;
> -- 
> 1.7.3.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] don't store temporary value returned from c_str()

[Tomi Ollila]

On Sat, Apr 27 2013, Jani Nikula <jani@nikula.org> wrote:

> On Sat, 20 Apr 2013, Vladimir.Marek@oracle.com wrote:
>> From: Vladimir Marek <vlmarek@volny.cz>
>>
>> This is causing problems when compiled by Oracle Studio. Memory pointed
>> by (const char*)term was already changed once talloc_strdup was called.
>>
>> Signed-off-by: Vladimir Marek <vlmarek@volny.cz>
>> ---
>>  lib/message.cc |    9 ++++-----
>>  1 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/message.cc b/lib/message.cc
>> index 8720c1b..8d329d1 100644
>> --- a/lib/message.cc
>> +++ b/lib/message.cc
>> @@ -266,18 +266,17 @@ _notmuch_message_get_term (notmuch_message_t *message,
>>  			   const char *prefix)
>>  {
>>      int prefix_len = strlen (prefix);
>> -    const char *term = NULL;
>>      char *value;
>>  
>>      i.skip_to (prefix);
>>  
>> -    if (i != end)
>> -	term = (*i).c_str ();
>
> It's okay to use the result of .c_str() as long as the string object
> stays in scope, and none of the non-const member functions are
> called. Here, I think the problem is that TermIterator's overloaded
> operator*() returns a string object within the if block's scope, and it
> goes immediately out of scope. You could check this by adding
>
> 	string s = *i;
>
> in function scope, and replacing (*i) with s in the if block. This might
> also be more obvious than the presented patch, but I think the patch is
> fine too.

Thanks, now I understand. I'd like to see updated patch using Jani's
example but I also think that the current patch is fine too.

>
> BR,
> Jani.

Tomi

>
>
>> +    if (i == end)
>> +	return NULL;
>>  
>> -    if (!term || strncmp (term, prefix, prefix_len))
>> +    if (strncmp ((*i).c_str(), prefix, prefix_len))
>>  	return NULL;
>>  
>> -    value = talloc_strdup (message, term + prefix_len);
>> +    value = talloc_strdup (message, (*i).c_str() + prefix_len);
>>  
>>  #if DEBUG_DATABASE_SANITY
>>      i++;
>> -- 
>> 1.7.3.2
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] don't store temporary value returned from c_str()

[Vladimir Marek]

> It's okay to use the result of .c_str() as long as the string object
> stays in scope, and none of the non-const member functions are
> called. Here, I think the problem is that TermIterator's overloaded
> operator*() returns a string object within the if block's scope, and it
> goes immediately out of scope. You could check this by adding

Right, I overlooked that TermIterator creates temporary string (if I
understand you correctly).

> 
> 	string s = *i;
>
> in function scope, and replacing (*i) with s in the if block. This might
> also be more obvious than the presented patch, but I think the patch is
> fine too.

I would prefer my change as it avoids creating another std::string. At
least I think.

Thank you
-- 
	Vlad

Re: [PATCH] don't store temporary value returned from c_str()

[David Bremner]

Vladimir Marek <Vladimir.Marek@Oracle.COM> writes:

>> It's okay to use the result of .c_str() as long as the string object
>> stays in scope, and none of the non-const member functions are
>> called. Here, I think the problem is that TermIterator's overloaded
>> operator*() returns a string object within the if block's scope, and it
>> goes immediately out of scope. You could check this by adding
>
> Right, I overlooked that TermIterator creates temporary string (if I
> understand you correctly).
>
>> fine too.
>
> I would prefer my change as it avoids creating another std::string. At
> least I think.

Would you mind updating your commit message to reflect the best guesses
from this discussion? 

d

Re: [PATCH] don't store temporary value returned from c_str()

[Vladimir Marek]

> Would you mind updating your commit message to reflect the best guesses
> from this discussion? 

Hi,

Sorry it took me a while. I just sent new version of the patch for
consideration.

-- 
	Vlad