[PATCH v3 0/2] Refactor config reading to support non-regular files

[PATCH v3 0/2] Refactor config reading to support non-regular files

[Ioan-Adrian Ratiu]

Changes since v2 (based on Tomi's feedback):
    * Rewrote config reading loop to use fread instead of fgets (behaves
    the same but the code looks much better now).

Ioan-Adrian Ratiu (1):
  notmuch-config: replace config reading function

Jani Nikula (1):
  cli: abstract config file reading to a separate function

 notmuch-config.c | 99 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 30 deletions(-)

-- 
2.10.2

[PATCH v3 1/2] cli: abstract config file reading to a separate function

[Ioan-Adrian Ratiu]

From: Jani Nikula <jani@nikula.org>

Simplify and fix the coding style while at it.
---
 notmuch-config.c | 65 ++++++++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index e5d42a0..bd52790 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -202,6 +202,38 @@ get_username_from_passwd_file (void *ctx)
     return name;
 }
 
+static notmuch_bool_t
+get_config_from_file (notmuch_config_t *config, notmuch_bool_t create_new)
+{
+    GError *error = NULL;
+    notmuch_bool_t ret = FALSE;
+
+    if (g_key_file_load_from_file (config->key_file, config->filename,
+				   G_KEY_FILE_KEEP_COMMENTS, &error))
+	return TRUE;
+
+    if (error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT) {
+	/* If create_new is true, then the caller is prepared for a
+	 * default configuration file in the case of FILE NOT FOUND.
+	 */
+	if (create_new) {
+	    config->is_new = TRUE;
+	    ret = TRUE;
+	} else {
+	    fprintf (stderr, "Configuration file %s not found.\n"
+		     "Try running 'notmuch setup' to create a configuration.\n",
+		     config->filename);
+	}
+    } else {
+	fprintf (stderr, "Error reading configuration file %s: %s\n",
+		 config->filename, error->message);
+    }
+
+    g_error_free (error);
+
+    return ret;
+}
+
 /* Open the named notmuch configuration file. If the filename is NULL,
  * the value of the environment variable $NOTMUCH_CONFIG will be used.
  * If $NOTMUCH_CONFIG is unset, the default configuration file
@@ -289,36 +321,9 @@ notmuch_config_open (void *ctx,
     config->search_exclude_tags_length = 0;
     config->crypto_gpg_path = NULL;
 
-    if (! g_key_file_load_from_file (config->key_file,
-				     config->filename,
-				     G_KEY_FILE_KEEP_COMMENTS,
-				     &error))
-    {
-	if (error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT) {
-	    /* If create_new is true, then the caller is prepared for a
-	     * default configuration file in the case of FILE NOT
-	     * FOUND.
-	     */
-	    if (create_new) {
-		g_error_free (error);
-		config->is_new = TRUE;
-	    } else {
-		fprintf (stderr, "Configuration file %s not found.\n"
-			 "Try running 'notmuch setup' to create a configuration.\n",
-			 config->filename);
-		talloc_free (config);
-		g_error_free (error);
-		return NULL;
-	    }
-	}
-	else
-	{
-	    fprintf (stderr, "Error reading configuration file %s: %s\n",
-		     config->filename, error->message);
-	    talloc_free (config);
-	    g_error_free (error);
-	    return NULL;
-	}
+    if (! get_config_from_file (config, create_new)) {
+	talloc_free (config);
+	return NULL;
     }
 
     /* Whenever we know of configuration sections that don't appear in
-- 
2.10.2

[PATCH v3 2/2] notmuch-config: replace config reading function

[Ioan-Adrian Ratiu]

Config files are currently read using glib's g_key_file_load_from_file
function which is very inconvenient because it's limited by design to read
only from "regular data files" in a filesystem. Because of this limitation
notmuch can't read configs from pipes, fifos, sockets, stdin, etc. Not even
"notmuch --config=/dev/stdin" works:

Error reading configuration file /dev/stdin: Not a regular file

So replace g_key_file_load_from_file with g_key_file_load_from_data which
gives us much more freedom to read configs from multiple sources.

This also helps the more security sensitive users: If someone has private
information in the config file, it can be encrypted on disk, then decrypted
in RAM and passed through a pipe directly to notmuch without the use of
intermediate plain text files.

Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
---
 notmuch-config.c | 60 ++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index bd52790..1ea897a 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -205,33 +205,67 @@ get_username_from_passwd_file (void *ctx)
 static notmuch_bool_t
 get_config_from_file (notmuch_config_t *config, notmuch_bool_t create_new)
 {
+    // #define BUF_SIZE 200 // test line (use debug code or strace(1) to verify)
+    #define BUF_SIZE 4096
+    char *config_str;
+    int config_len = 0;
+    int config_bufsize = BUF_SIZE;
+    size_t len;
     GError *error = NULL;
-    notmuch_bool_t ret = FALSE;
 
-    if (g_key_file_load_from_file (config->key_file, config->filename,
-				   G_KEY_FILE_KEEP_COMMENTS, &error))
-	return TRUE;
-
-    if (error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT) {
+    FILE *fp = fopen(config->filename, "r");
+    if (fp == NULL) {
 	/* If create_new is true, then the caller is prepared for a
 	 * default configuration file in the case of FILE NOT FOUND.
 	 */
 	if (create_new) {
 	    config->is_new = TRUE;
-	    ret = TRUE;
+	    return TRUE;
 	} else {
-	    fprintf (stderr, "Configuration file %s not found.\n"
+	    fprintf (stderr, "Error opening config file '%s': %s\n"
 		     "Try running 'notmuch setup' to create a configuration.\n",
-		     config->filename);
+		     config->filename, strerror(errno));
+	    return FALSE;
 	}
-    } else {
-	fprintf (stderr, "Error reading configuration file %s: %s\n",
-		 config->filename, error->message);
     }
 
+    config_str = talloc_zero_array (config, char, config_bufsize);
+    if (config_str == NULL) {
+	fprintf (stderr, "Error reading '%s': Out of memory\n", config->filename);
+	return FALSE;
+    }
+
+    while ((len = fread (config_str + config_len, 1,
+			 config_bufsize - config_len, fp)) > 0) {
+	config_len += len;
+	if (config_len == config_bufsize) {
+	    config_bufsize += BUF_SIZE;
+	    config_str = talloc_realloc (config, config_str, char, config_bufsize);
+	    if (config_str == NULL) {
+		fprintf (stderr, "Error reading '%s': Failed to reallocate memory\n",
+			 config->filename);
+		return FALSE;
+	    }
+	}
+    }
+
+    if (ferror (fp)) {
+	fprintf (stderr, "Error reading '%s': I/O error\n", config->filename);
+	return FALSE;
+    }
+
+    fclose(fp);
+
+    if (g_key_file_load_from_data (config->key_file, config_str, strlen(config_str),
+				   G_KEY_FILE_KEEP_COMMENTS, &error))
+	return TRUE;
+
+    fprintf (stderr, "Error parsing config file '%s': %s\n",
+	     config->filename, error->message);
+
     g_error_free (error);
 
-    return ret;
+    return FALSE;
 }
 
 /* Open the named notmuch configuration file. If the filename is NULL,
-- 
2.10.2

Re: [PATCH v3 2/2] notmuch-config: replace config reading function

[Tomi Ollila]

On Sun, Dec 04 2016, Ioan-Adrian Ratiu <adi@adirat.com> wrote:

> Config files are currently read using glib's g_key_file_load_from_file
> function which is very inconvenient because it's limited by design to read
> only from "regular data files" in a filesystem. Because of this limitation
> notmuch can't read configs from pipes, fifos, sockets, stdin, etc. Not even
> "notmuch --config=/dev/stdin" works:
>
> Error reading configuration file /dev/stdin: Not a regular file
>
> So replace g_key_file_load_from_file with g_key_file_load_from_data which
> gives us much more freedom to read configs from multiple sources.
>
> This also helps the more security sensitive users: If someone has private
> information in the config file, it can be encrypted on disk, then decrypted
> in RAM and passed through a pipe directly to notmuch without the use of
> intermediate plain text files.

That was QUICK! I try to do a bit more through review this time ;)

>
> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
> ---
>  notmuch-config.c | 60 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index bd52790..1ea897a 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -205,33 +205,67 @@ get_username_from_passwd_file (void *ctx)
>  static notmuch_bool_t
>  get_config_from_file (notmuch_config_t *config, notmuch_bool_t create_new)
>  {
> +    // #define BUF_SIZE 200 // test line (use debug code or strace(1) to verify)

(actually I thought the above line to be used while testing the code before
submission, and delete before that... ;)

> +    #define BUF_SIZE 4096
> +    char *config_str;

Hmm for reasons seen below, better do char *config_str = NULL (i.e. not
like my previous suggestion) although not necessary it is a bit safer
for future editors to mess with -- IMO we should use a bit more RAII way
and declare the variable where it is first used (c99-supported feature),
but that is not current status quo (or someone(tm) may know better why
that would not be desired...)

> +    int config_len = 0;
> +    int config_bufsize = BUF_SIZE;
> +    size_t len;
>      GError *error = NULL;
> -    notmuch_bool_t ret = FALSE;
>  
> -    if (g_key_file_load_from_file (config->key_file, config->filename,
> -				   G_KEY_FILE_KEEP_COMMENTS, &error))
> -	return TRUE;
> -
> -    if (error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT) {
> +    FILE *fp = fopen(config->filename, "r");
> +    if (fp == NULL) {
>  	/* If create_new is true, then the caller is prepared for a
>  	 * default configuration file in the case of FILE NOT FOUND.
>  	 */
>  	if (create_new) {
>  	    config->is_new = TRUE;
> -	    ret = TRUE;
> +	    return TRUE;
>  	} else {
> -	    fprintf (stderr, "Configuration file %s not found.\n"
> +	    fprintf (stderr, "Error opening config file '%s': %s\n"
>  		     "Try running 'notmuch setup' to create a configuration.\n",
> -		     config->filename);
> +		     config->filename, strerror(errno));
> +	    return FALSE;
>  	}
> -    } else {
> -	fprintf (stderr, "Error reading configuration file %s: %s\n",
> -		 config->filename, error->message);
>      }
>  
> +    config_str = talloc_zero_array (config, char, config_bufsize);
> +    if (config_str == NULL) {
> +	fprintf (stderr, "Error reading '%s': Out of memory\n", config->filename);

close fp -- or better, set `ret` and goto out; (or if ret is (first
declared, and) initialized to FALSE, then no need to set).

> +	return FALSE;
> +    }
> +
> +    while ((len = fread (config_str + config_len, 1,
> +			 config_bufsize - config_len, fp)) > 0) {
> +	config_len += len;
> +	if (config_len == config_bufsize) {
> +	    config_bufsize += BUF_SIZE;
> +	    config_str = talloc_realloc (config, config_str, char, config_bufsize);
> +	    if (config_str == NULL) {
> +		fprintf (stderr, "Error reading '%s': Failed to reallocate memory\n",
> +			 config->filename);

ditto

> +		return FALSE;
> +	    }
> +	}
> +    }
> +
> +    if (ferror (fp)) {
> +	fprintf (stderr, "Error reading '%s': I/O error\n", config->filename);

ditto

> +	return FALSE;
> +    }
> +
> +    fclose(fp);

remove above...

> +
> +    if (g_key_file_load_from_data (config->key_file, config_str, strlen(config_str),

here instead of strlen(), use config_len -- strlen() will stop at first
embedded '\0' in data.

> +				   G_KEY_FILE_KEEP_COMMENTS, &error))
> +	return TRUE;

set `ret` to TRUE, goto out;

> +
> +    fprintf (stderr, "Error parsing config file '%s': %s\n",
> +	     config->filename, error->message);
> +
>      g_error_free (error);

out:
  close(fp);
  if (config_str) talloc_free(config_str);
  return ret;

>  
> -    return ret;
> +    return FALSE;
>  }
>  
>  /* Open the named notmuch configuration file. If the filename is NULL,
> -- 
> 2.10.2

Re: [PATCH v3 2/2] notmuch-config: replace config reading function

[Jani Nikula]

On Sun, 04 Dec 2016, Ioan-Adrian Ratiu <adi@adirat.com> wrote:
> Config files are currently read using glib's g_key_file_load_from_file
> function which is very inconvenient because it's limited by design to read
> only from "regular data files" in a filesystem. Because of this limitation
> notmuch can't read configs from pipes, fifos, sockets, stdin, etc. Not even
> "notmuch --config=/dev/stdin" works:
>
> Error reading configuration file /dev/stdin: Not a regular file
>
> So replace g_key_file_load_from_file with g_key_file_load_from_data which
> gives us much more freedom to read configs from multiple sources.
>
> This also helps the more security sensitive users: If someone has private
> information in the config file, it can be encrypted on disk, then decrypted
> in RAM and passed through a pipe directly to notmuch without the use of
> intermediate plain text files.
>
> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
> ---
>  notmuch-config.c | 60 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index bd52790..1ea897a 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -205,33 +205,67 @@ get_username_from_passwd_file (void *ctx)
>  static notmuch_bool_t
>  get_config_from_file (notmuch_config_t *config, notmuch_bool_t create_new)
>  {
> +    // #define BUF_SIZE 200 // test line (use debug code or strace(1) to verify)
> +    #define BUF_SIZE 4096
> +    char *config_str;
> +    int config_len = 0;
> +    int config_bufsize = BUF_SIZE;
> +    size_t len;
>      GError *error = NULL;
> -    notmuch_bool_t ret = FALSE;
>  
> -    if (g_key_file_load_from_file (config->key_file, config->filename,
> -				   G_KEY_FILE_KEEP_COMMENTS, &error))
> -	return TRUE;
> -
> -    if (error->domain == G_FILE_ERROR && error->code == G_FILE_ERROR_NOENT) {
> +    FILE *fp = fopen(config->filename, "r");
> +    if (fp == NULL) {
>  	/* If create_new is true, then the caller is prepared for a
>  	 * default configuration file in the case of FILE NOT FOUND.
>  	 */
>  	if (create_new) {
>  	    config->is_new = TRUE;
> -	    ret = TRUE;
> +	    return TRUE;
>  	} else {
> -	    fprintf (stderr, "Configuration file %s not found.\n"
> +	    fprintf (stderr, "Error opening config file '%s': %s\n"
>  		     "Try running 'notmuch setup' to create a configuration.\n",
> -		     config->filename);
> +		     config->filename, strerror(errno));
> +	    return FALSE;
>  	}
> -    } else {
> -	fprintf (stderr, "Error reading configuration file %s: %s\n",
> -		 config->filename, error->message);
>      }
>  
> +    config_str = talloc_zero_array (config, char, config_bufsize);
> +    if (config_str == NULL) {
> +	fprintf (stderr, "Error reading '%s': Out of memory\n", config->filename);
> +	return FALSE;
> +    }
> +
> +    while ((len = fread (config_str + config_len, 1,
> +			 config_bufsize - config_len, fp)) > 0) {
> +	config_len += len;
> +	if (config_len == config_bufsize) {
> +	    config_bufsize += BUF_SIZE;
> +	    config_str = talloc_realloc (config, config_str, char, config_bufsize);
> +	    if (config_str == NULL) {
> +		fprintf (stderr, "Error reading '%s': Failed to reallocate memory\n",
> +			 config->filename);
> +		return FALSE;
> +	    }
> +	}
> +    }
> +
> +    if (ferror (fp)) {
> +	fprintf (stderr, "Error reading '%s': I/O error\n", config->filename);
> +	return FALSE;
> +    }
> +
> +    fclose(fp);
> +
> +    if (g_key_file_load_from_data (config->key_file, config_str, strlen(config_str),
> +				   G_KEY_FILE_KEEP_COMMENTS, &error))

I think you should free config_str regardless of the
g_key_file_load_from_data() return value.

> +	return TRUE;
> +
> +    fprintf (stderr, "Error parsing config file '%s': %s\n",
> +	     config->filename, error->message);
> +
>      g_error_free (error);
>  
> -    return ret;
> +    return FALSE;
>  }
>  
>  /* Open the named notmuch configuration file. If the filename is NULL,
> -- 
> 2.10.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch