From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id A5FCA6DE0EF3 for ; Mon, 25 Nov 2019 09:21:34 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: 0.398 X-Spam-Level: X-Spam-Status: No, score=0.398 tagged_above=-999 required=5 tests=[AWL=-0.254, SPF_NEUTRAL=0.652] autolearn=disabled Received: from arlo.cworth.org ([127.0.0.1]) by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ih7UTvyo1YLt for ; Mon, 25 Nov 2019 09:21:32 -0800 (PST) Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) by arlo.cworth.org (Postfix) with ESMTP id 8584D6DE0EED for ; Mon, 25 Nov 2019 09:21:30 -0800 (PST) Received: from guru.guru-group.fi (localhost [IPv6:::1]) by guru.guru-group.fi (Postfix) with ESMTP id 783561000D0; Mon, 25 Nov 2019 19:21:25 +0200 (EET) From: Tomi Ollila To: David Bremner , notmuch@notmuchmail.org Subject: Re: [PATCH] lib: fix memory error in notmuch_config_list_value In-Reply-To: <20191125023734.1235130-1-david@tethera.net> References: <20191125023734.1235130-1-david@tethera.net> User-Agent: Notmuch/0.28.3+84~g41389bb (https://notmuchmail.org) Emacs/25.2.1 (x86_64-unknown-linux-gnu) X-Face: HhBM'cA~ MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Nov 2019 17:21:34 -0000 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) Tomi > > /* TODO: better error reporting?? */ > status = _metadata_value (list->notmuch, key, strval); > @@ -177,6 +181,7 @@ notmuch_config_list_value (notmuch_config_list_t *list) > talloc_free (list->current_val); > > list->current_val = talloc_strdup (list, strval.c_str ()); > + talloc_free (key); > return list->current_val; > } > > -- > 2.24.0 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch