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

[Tomi Ollila]

On Mon, Dec 05 2016, Ioan-Adrian Ratiu  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 
> ---
>  notmuch-config.c | 58 
> ++--
>  1 file changed, 48 insertions(+), 10 deletions(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index bd52790..fe16fa3 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -205,32 +205,70 @@ 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 4096
> +char *config_str = NULL;
> +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, ))
> - 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;
> + goto out;
>   } 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));
> + goto out;
>   }
> -} 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);
> + goto out;
> +}
> +
> +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);
> + goto out;
> + }
> + }
> +}
> +
> +if (ferror (fp)) {
> + fprintf (stderr, "Error reading '%s': I/O error\n", config->filename);
> + goto out;
> +}
> +
> +if (g_key_file_load_from_data (config->key_file, config_str, 
> strlen(config_str),

How sticky can this strlen() be ? config_len !

> +G_KEY_FILE_KEEP_COMMENTS, )) {
> + ret = TRUE;
> + goto out;
> +}
> +
> +fprintf (stderr, "Error parsing config file '%s': %s\n",
> +  config->filename, error->message);
> +
>  g_error_free (error);
>  
> +out:
> +fclose(fp);

need to check that fp != NULL -- fclose() manual page does not state
calling fclose(NULL) is supported.

> +if (config_str) talloc_free(config_str);

You copied my code verbatim ;/ better write if() and talloc_free() to
separate lines (also fclose() when updated)

no other comments from me.

Tomi

>  return ret;
>  }
>  
> -- 
> 2.10.2
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v4 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 
---
 notmuch-config.c | 58 ++--
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index bd52790..fe16fa3 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -205,32 +205,70 @@ 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 4096
+char *config_str = NULL;
+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, ))
-   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;
+   goto out;
} 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));
+   goto out;
}
-} 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);
+   goto out;
+}
+
+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);
+   goto out;
+   }
+   }
+}
+
+if (ferror (fp)) {
+   fprintf (stderr, "Error reading '%s': I/O error\n", config->filename);
+   goto out;
+}
+
+if (g_key_file_load_from_data (config->key_file, config_str, 
strlen(config_str),
+  G_KEY_FILE_KEEP_COMMENTS, )) {
+   ret = TRUE;
+   goto out;
+}
+
+fprintf (stderr, "Error parsing config file '%s': %s\n",
+config->filename, error->message);
+
 g_error_free (error);
 
+out:
+fclose(fp);
+if (config_str) talloc_free(config_str);
 return ret;
 }
 
-- 
2.10.2

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch