unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH] don't store temporary value returned from c_str()
@ 2013-04-19 21:12 Vladimir.Marek
  2013-04-27  9:33 ` Tomi Ollila
  2013-04-27 12:30 ` Jani Nikula
  0 siblings, 2 replies; 14+ messages in thread
From: Vladimir.Marek @ 2013-04-19 21:12 UTC (permalink / raw)
  To: notmuch; +Cc: 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-19 21:12 [PATCH] don't store temporary value returned from c_str() Vladimir.Marek
@ 2013-04-27  9:33 ` Tomi Ollila
  2013-04-27 10:11   ` Vladimir Marek
  2013-04-27 12:30 ` Jani Nikula
  1 sibling, 1 reply; 14+ messages in thread
From: Tomi Ollila @ 2013-04-27  9:33 UTC (permalink / raw)
  To: Vladimir.Marek, notmuch; +Cc: Vladimir Marek

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-27  9:33 ` Tomi Ollila
@ 2013-04-27 10:11   ` Vladimir Marek
  2013-04-27 11:53     ` David Bremner
  2013-04-30  6:12     ` Kim Minh Kaplan
  0 siblings, 2 replies; 14+ messages in thread
From: Vladimir Marek @ 2013-04-27 10:11 UTC (permalink / raw)
  To: Tomi Ollila; +Cc: notmuch, 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-27 10:11   ` Vladimir Marek
@ 2013-04-27 11:53     ` David Bremner
  2013-04-27 15:59       ` Vladimir Marek
  2013-04-30  6:12     ` Kim Minh Kaplan
  1 sibling, 1 reply; 14+ messages in thread
From: David Bremner @ 2013-04-27 11:53 UTC (permalink / raw)
  To: Vladimir Marek, Tomi Ollila; +Cc: notmuch, Vladimir Marek

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-19 21:12 [PATCH] don't store temporary value returned from c_str() Vladimir.Marek
  2013-04-27  9:33 ` Tomi Ollila
@ 2013-04-27 12:30 ` Jani Nikula
  2013-04-27 13:22   ` Tomi Ollila
  2013-04-27 16:05   ` Vladimir Marek
  1 sibling, 2 replies; 14+ messages in thread
From: Jani Nikula @ 2013-04-27 12:30 UTC (permalink / raw)
  To: Vladimir.Marek, notmuch; +Cc: Vladimir Marek

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-27 12:30 ` Jani Nikula
@ 2013-04-27 13:22   ` Tomi Ollila
  2013-04-27 16:05   ` Vladimir Marek
  1 sibling, 0 replies; 14+ messages in thread
From: Tomi Ollila @ 2013-04-27 13:22 UTC (permalink / raw)
  To: Jani Nikula, Vladimir.Marek, notmuch; +Cc: Vladimir Marek

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-27 11:53     ` David Bremner
@ 2013-04-27 15:59       ` Vladimir Marek
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Marek @ 2013-04-27 15:59 UTC (permalink / raw)
  To: notmuch

> > 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-27 12:30 ` Jani Nikula
  2013-04-27 13:22   ` Tomi Ollila
@ 2013-04-27 16:05   ` Vladimir Marek
  2013-04-29 21:23     ` David Bremner
  1 sibling, 1 reply; 14+ messages in thread
From: Vladimir Marek @ 2013-04-27 16:05 UTC (permalink / raw)
  To: Jani Nikula; +Cc: notmuch, 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-27 16:05   ` Vladimir Marek
@ 2013-04-29 21:23     ` David Bremner
  2013-05-01 21:33       ` Vladimir Marek
  0 siblings, 1 reply; 14+ messages in thread
From: David Bremner @ 2013-04-29 21:23 UTC (permalink / raw)
  To: Vladimir Marek, Jani Nikula; +Cc: notmuch

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-27 10:11   ` Vladimir Marek
  2013-04-27 11:53     ` David Bremner
@ 2013-04-30  6:12     ` Kim Minh Kaplan
  2013-04-30  8:48       ` Vladimir Marek
  1 sibling, 1 reply; 14+ messages in thread
From: Kim Minh Kaplan @ 2013-04-30  6:12 UTC (permalink / raw)
  To: Tomi Ollila; +Cc: notmuch, Vladimir Marek

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-30  6:12     ` Kim Minh Kaplan
@ 2013-04-30  8:48       ` Vladimir Marek
  2013-04-30  9:44         ` Kim Minh Kaplan
  0 siblings, 1 reply; 14+ messages in thread
From: Vladimir Marek @ 2013-04-30  8:48 UTC (permalink / raw)
  To: Kim Minh Kaplan; +Cc: Tomi Ollila, notmuch, 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;
}

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-30  8:48       ` Vladimir Marek
@ 2013-04-30  9:44         ` Kim Minh Kaplan
  2013-05-01 11:28           ` Vladimir Marek
  0 siblings, 1 reply; 14+ messages in thread
From: Kim Minh Kaplan @ 2013-04-30  9:44 UTC (permalink / raw)
  To: Vladimir Marek; +Cc: Tomi Ollila, notmuch

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-30  9:44         ` Kim Minh Kaplan
@ 2013-05-01 11:28           ` Vladimir Marek
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Marek @ 2013-05-01 11:28 UTC (permalink / raw)
  To: Kim Minh Kaplan; +Cc: Tomi Ollila, notmuch, 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] don't store temporary value returned from c_str()
  2013-04-29 21:23     ` David Bremner
@ 2013-05-01 21:33       ` Vladimir Marek
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Marek @ 2013-05-01 21:33 UTC (permalink / raw)
  To: David Bremner; +Cc: notmuch

> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2013-05-01 21:35 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-19 21:12 [PATCH] don't store temporary value returned from c_str() Vladimir.Marek
2013-04-27  9:33 ` Tomi Ollila
2013-04-27 10:11   ` Vladimir Marek
2013-04-27 11:53     ` David Bremner
2013-04-27 15:59       ` Vladimir Marek
2013-04-30  6:12     ` Kim Minh Kaplan
2013-04-30  8:48       ` Vladimir Marek
2013-04-30  9:44         ` Kim Minh Kaplan
2013-05-01 11:28           ` Vladimir Marek
2013-04-27 12:30 ` Jani Nikula
2013-04-27 13:22   ` Tomi Ollila
2013-04-27 16:05   ` Vladimir Marek
2013-04-29 21:23     ` David Bremner
2013-05-01 21:33       ` Vladimir Marek

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).