Date: Tue, 01 Dec 2009 20:50:03 +0200
Message-ID: <87638qbkbo.wl%djcb@djcbsoftware.nl>
From: Dirk-Jan C. Binnema <djcb.bulk@gmail.com>
To: Carl Worth <cworth@cworth.org>
In-Reply-To: <87k4x6ad94.fsf@yoom.home.cworth.org>
References: <87r5rpyd4x.wl%djcb@djcbsoftware.nl>
	<87k4x6ad94.fsf@yoom.home.cworth.org>
Mail-Reply-To: djcb@djcbsoftware.nl
User-Agent: Wanderlust/2.15.6 (Almost Unreal) Emacs/23.1 Mule/6.0
	(HANACHIRUSATO)
Organization: DJCBSoftware
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: multipart/mixed; boundary="Multipart_Tue_Dec__1_20:50:03_2009-1"
Content-Transfer-Encoding: 7bit
Cc: "notmuch@notmuchmail org" <notmuch@notmuchmail.org>
Subject: Re: [notmuch] [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to
	ignored 'fflush' return value
Precedence: list
Reply-To: djcb@djcbsoftware.nl

--Multipart_Tue_Dec__1_20:50:03_2009-1
Content-Type: text/plain; charset=US-ASCII

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.


--Multipart_Tue_Dec__1_20:50:03_2009-1
Content-Type: application/octet-stream; type=patch
Content-Disposition: attachment;
 filename="0001-notmuch-config-fix-small-leak-from-g_key_file_to_dat.patch"
Content-Transfer-Encoding: 7bit

---
 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


--Multipart_Tue_Dec__1_20:50:03_2009-1
Content-Type: text/plain; charset=US-ASCII




--Multipart_Tue_Dec__1_20:50:03_2009-1
Content-Type: application/octet-stream; type=patch
Content-Disposition: attachment;
 filename="0002-free-the-response-data-from-prompt.patch"
Content-Transfer-Encoding: 7bit

---
 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


--Multipart_Tue_Dec__1_20:50:03_2009-1
Content-Type: text/plain; charset=US-ASCII




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


--Multipart_Tue_Dec__1_20:50:03_2009-1--