[PATCH] lib: fix memory error in notmuch_config_list_value

[PATCH] lib: fix memory error in notmuch_config_list_value

[David Bremner]

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);
 
     /* 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

Re: [PATCH] lib: fix memory error in notmuch_config_list_value

[Tomi Ollila]

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

Re: [PATCH] lib: fix memory error in notmuch_config_list_value

[Daniel Kahn Gillmor]

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