[PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value

[PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value

[Dirk-Jan C. Binnema]

---
 notmuch-setup.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/notmuch-setup.c b/notmuch-setup.c
index d06fbf8..c50f812 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -100,12 +100,13 @@ notmuch_setup_command (unused (void *ctx),
     unsigned int i;
     int is_new;
 
-#define prompt(format, ...)				\
-    do {						\
-	printf (format, ##__VA_ARGS__);			\
-	fflush (stdout);				\
-	getline (&response, &response_size, stdin);	\
-	chomp_newline (response);			\
+#define prompt(format, ...)					\
+    do {							\
+	int ignored;						\
+	printf (format, ##__VA_ARGS__);				\
+	fflush (stdout);					\
+	ignored = getline (&response, &response_size, stdin);	\
+	chomp_newline (response);				\
     } while (0)
 
     config = notmuch_config_open (ctx, NULL, &is_new);
-- 
1.6.3.3

Re: [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2657 bytes --]

On Mon, 23 Nov 2009 08:21:50 +0200, Dirk-Jan C. Binnema <djcb.bulk@gmail.com> wrote:
> -#define prompt(format, ...)				\
> -    do {						\
> -	printf (format, ##__VA_ARGS__);			\
> -	fflush (stdout);				\
> -	getline (&response, &response_size, stdin);	\
> -	chomp_newline (response);			\
> +#define prompt(format, ...)					\
> +    do {							\
> +	int ignored;						\
> +	printf (format, ##__VA_ARGS__);				\
> +	fflush (stdout);					\
> +	ignored = getline (&response, &response_size, stdin);	\
> +	chomp_newline (response);				\
>      } while (0)

This patch is incorrect. Ignoring the return value of getline results in
the program invoking undefined behavior by reading uninitialized
memory. This is easily tested by, for example, typing Control-D to
provide EOF to a prompt from "notmuch setup".

How about just exiting in case of EOF as in the patch below?

I think I'll push it now.

-Carl

commit eb0cf86c7a9d5cda464d4d36a9cac66f26b5529d
Author: Carl Worth <cworth@cworth.org>
Date:   Tue Dec 1 08:06:09 2009 -0800

    notmuch setup: Exit if EOF is encountered at any prompt.
    
    If the user is explicitly providing EOF, then terminating the program
    is the most likely desired thing to do. This also avoids undefined
    behavior from continuing with an uninitialized response after ignoring
    the return value of getline().

diff --git a/notmuch-setup.c b/notmuch-setup.c
index 5ec176d..622bbaa 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -100,12 +100,15 @@ notmuch_setup_command (unused (void *ctx),
     unsigned int i;
     int is_new;
 
-#define prompt(format, ...)                            \
-    do {                                               \
-       printf (format, ##__VA_ARGS__);                 \
-       fflush (stdout);                                \
-       getline (&response, &response_size, stdin);     \
-       chomp_newline (response);                       \
+#define prompt(format, ...)                                    \
+    do {                                                       \
+       printf (format, ##__VA_ARGS__);                         \
+       fflush (stdout);                                        \
+       if (getline (&response, &response_size, stdin) < 0) {   \
+           printf ("Exiting.\n");                              \
+           exit (1);                                           \
+       }                                                       \
+       chomp_newline (response);                               \
     } while (0)
 
     config = notmuch_config_open (ctx, NULL, &is_new);

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value

[Dirk-Jan C. Binnema]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

Hi Carl,

>>>>> "Carl" == Carl Worth <cworth@cworth.org> writes:

    Carl> [1  <text/plain (quoted-printable)>]
    Carl> On Mon, 23 Nov 2009 08:21:50 +0200, Dirk-Jan C. Binnema <djcb.bulk@gmail.com> wrote:
    >> -#define prompt(format, ...)				\
    >> -    do {						\
    >> -	printf (format, ##__VA_ARGS__);			\
    >> -	fflush (stdout);				\
    >> -	getline (&response, &response_size, stdin);	\
    >> -	chomp_newline (response);			\
    >> +#define prompt(format, ...)					\
    >> +    do {							\
    >> +	int ignored;						\
    >> +	printf (format, ##__VA_ARGS__);				\
    >> +	fflush (stdout);					\
    >> +	ignored = getline (&response, &response_size, stdin);	\
    >> +	chomp_newline (response);				\
    >> } while (0)

    Carl> This patch is incorrect. Ignoring the return value of getline results in
    Carl> the program invoking undefined behavior by reading uninitialized
    Carl> memory. This is easily tested by, for example, typing Control-D to
    Carl> provide EOF to a prompt from "notmuch setup".

    Carl> How about just exiting in case of EOF as in the patch below?

Sure, that's the better solution, but note that my patch did not introduce the
undefined behavior -- it was there before. I was trying a minimal patch to
silencing the warning. Note that prompt seems to leak a bit, even after the
committed patch; attached are two more micro patches to fix this and another
small leak. I try to do minimal changes, but the prompt business gets a bit
unwieldy. The leaks are one-time at not critical, but anyway it's always good
stay vigilant.


[-- Attachment #2: 0001-notmuch-config-fix-small-leak-from-g_key_file_to_dat.patch --]
[-- Type: application/octet-stream, Size: 491 bytes --]

---
 notmuch-config.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index fc65d6b..95430db 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -317,9 +317,11 @@ notmuch_config_save (notmuch_config_t *config)
 	fprintf (stderr, "Error saving configuration to %s: %s\n",
 		 config->filename, error->message);
 	g_error_free (error);
+	g_free (data);
 	return 1;
     }
 
+    g_free (data);
     return 0;
 }
 
-- 
1.6.3.3


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]




[-- Attachment #4: 0002-free-the-response-data-from-prompt.patch --]
[-- Type: application/octet-stream, Size: 1674 bytes --]

---
 notmuch-setup.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/notmuch-setup.c b/notmuch-setup.c
index 622bbaa..293c852 100644
--- a/notmuch-setup.c
+++ b/notmuch-setup.c
@@ -119,12 +119,16 @@ notmuch_setup_command (unused (void *ctx),
     prompt ("Your full name [%s]: ", notmuch_config_get_user_name (config));
     if (strlen (response))
 	notmuch_config_set_user_name (config, response);
-
+    free (response);
+    response = NULL;
+    
     prompt ("Your primary email address [%s]: ",
 	    notmuch_config_get_user_primary_email (config));
     if (strlen (response))
 	notmuch_config_set_user_primary_email (config, response);
-
+    free (response);
+    response = NULL;
+    
     other_emails = g_ptr_array_new ();
 
     old_other_emails = notmuch_config_get_user_other_email (config,
@@ -136,12 +140,17 @@ notmuch_setup_command (unused (void *ctx),
 	else
 	    g_ptr_array_add (other_emails, talloc_strdup (ctx,
 							 old_other_emails[i]));
+	free (response);
+	response = NULL;
     }
 
     do {
 	prompt ("Additional email address [Press 'Enter' if none]: ");
 	if (strlen (response))
 	    g_ptr_array_add (other_emails, talloc_strdup (ctx, response));
+	free (response);
+	response = NULL;
+	
     } while (strlen (response));
     if (other_emails->len)
 	notmuch_config_set_user_other_email (config,
@@ -158,6 +167,8 @@ notmuch_setup_command (unused (void *ctx),
 	absolute_path = make_path_absolute (ctx, response);
 	notmuch_config_set_database_path (config, absolute_path);
     }
+    free (response);
+    response = NULL;
 
     if (! notmuch_config_save (config)) {
 	if (is_new)
-- 
1.6.3.3


[-- Attachment #5: Type: text/plain, Size: 193 bytes --]




Best wishes,
Dirk.

-- 
Dirk-Jan C. Binnema                  Helsinki, Finland
e:djcb@djcbsoftware.nl           w:www.djcbsoftware.nl
pgp: D09C E664 897D 7D39 5047 A178 E96A C7A1 017D DA3C

Re: [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

On Tue, 01 Dec 2009 20:50:03 +0200, Dirk-Jan C. Binnema <djcb.bulk@gmail.com> wrote:
> Sure, that's the better solution, but note that my patch did not introduce the
> undefined behavior -- it was there before. I was trying a minimal patch to
> silencing the warning.

Yes, the leak was my bug.

And the warning was useful to point to the bug. That's why silencing it
would have been a bad thing.

> Note that prompt seems to leak a bit, even after the
> committed patch; attached are two more micro patches to fix this and another
> small leak. I try to do minimal changes, but the prompt business gets a bit
> unwieldy. The leaks are one-time at not critical, but anyway it's always good
> stay vigilant.

Yes, leak fixes are always appreciated.

Could you resend these as complete git commits? I see only git diffs
which lack both authorship and commit messages.

Also, if you can send patches as text/plain rather than
application/octet-stream then it's much easier to review them by simply
replying and having the patch be quoted in the reply.

Thanks,

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]