BUG: Using pointer that points to a destructed string's content
Tamas Szakaly 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
Re: BUG: Using pointer that points to a destructed string's content
Tamas Szakaly 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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: BUG: Using pointer that points to a destructed string's content
-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.cc2014-06-25 12:30:10.0
+0200
+++ /root/message.cc2014-12-20 08:15:10.0 +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-
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
BUG: Using pointer that points to a destructed string's content
-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.cc2014-06-25 12:30:10.0
+0200
+++ /root/message.cc2014-12-20 08:15:10.0 +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-
BUG: Using pointer that points to a destructed string's content
Tamas Szakaly 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 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");
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
BUG: Using pointer that points to a destructed string's content
-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-
BUG: Using pointer that points to a destructed string's content
-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- ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
