From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
To: Tomi Ollila <tomi.ollila@iki.fi>,
David Bremner <david@tethera.net>,
notmuch@notmuchmail.org
Subject: Re: [PATCH] lib: fix memory error in notmuch_config_list_value
Date: Mon, 09 Dec 2019 15:31:42 -0500 [thread overview]
Message-ID: <878snl8clt.fsf@fifthhorseman.net> (raw)
In-Reply-To: <m2h82r98iz.fsf@guru.guru-group.fi>
[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]
On Mon 2019-11-25 19:21:24 +0200, Tomi Ollila wrote:
> On Sun, Nov 24 2019, David Bremner wrote:
>
>> The documentation for notmuch_config_list_key warns that that the
>> returned value will be destroyed by the next call to
>> notmuch_config_list_key, but it neglected to mention that calling
>> notmuch_config_list_value would also destroy it (by calling
>> notmuch_config_list_key). This is surprising, and caused a use after
>> free bug in _setup_user_query_fields (first noticed by an OpenBSD
>> porter, so kudos to the OpenBSD malloc implementation). This change
>> fixes that use-after-free bug.
>> ---
>> lib/config.cc | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/config.cc b/lib/config.cc
>> index da71c16e..2cd8a0b6 100644
>> --- a/lib/config.cc
>> +++ b/lib/config.cc
>> @@ -150,13 +150,17 @@ notmuch_config_list_valid (notmuch_config_list_t *metadata)
>> return true;
>> }
>>
>> +static inline char * _key_from_iterator (notmuch_config_list_t *list) {
>> + return talloc_strdup (list, (*list->iterator).c_str () + CONFIG_PREFIX.length ());
>> +}
>> +
>> const char *
>> notmuch_config_list_key (notmuch_config_list_t *list)
>> {
>> if (list->current_key)
>> talloc_free (list->current_key);
>>
>> - list->current_key = talloc_strdup (list, (*list->iterator).c_str () + CONFIG_PREFIX.length ());
>> + list->current_key = _key_from_iterator (list);
>>
>> return list->current_key;
>> }
>> @@ -166,7 +170,7 @@ notmuch_config_list_value (notmuch_config_list_t *list)
>> {
>> std::string strval;
>> notmuch_status_t status;
>> - const char *key = notmuch_config_list_key (list);
>> + char *key = _key_from_iterator (list);
>
> 2 spaces, otherwise looks good (on paper)
I concur with Tomi. Please clean up the 2 spaces, and merge!
--dkg
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]
prev parent reply other threads:[~2019-12-09 22:39 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-25 2:37 [PATCH] lib: fix memory error in notmuch_config_list_value David Bremner
2019-11-25 17:21 ` Tomi Ollila
2019-12-09 20:31 ` Daniel Kahn Gillmor [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://notmuchmail.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=878snl8clt.fsf@fifthhorseman.net \
--to=dkg@fifthhorseman.net \
--cc=david@tethera.net \
--cc=notmuch@notmuchmail.org \
--cc=tomi.ollila@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).