* [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value @ 2009-11-23 6:21 Dirk-Jan C. Binnema 2009-12-01 16:08 ` Carl Worth 0 siblings, 1 reply; 4+ messages in thread From: Dirk-Jan C. Binnema @ 2009-11-23 6:21 UTC (permalink / raw) To: notmuch@notmuchmail org --- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value 2009-11-23 6:21 [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value Dirk-Jan C. Binnema @ 2009-12-01 16:08 ` Carl Worth 2009-12-01 18:50 ` Dirk-Jan C. Binnema 0 siblings, 1 reply; 4+ messages in thread From: Carl Worth @ 2009-12-01 16:08 UTC (permalink / raw) To: djcb, notmuch@notmuchmail org [-- 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 --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value 2009-12-01 16:08 ` Carl Worth @ 2009-12-01 18:50 ` Dirk-Jan C. Binnema 2009-12-02 3:01 ` Carl Worth 0 siblings, 1 reply; 4+ messages in thread From: Dirk-Jan C. Binnema @ 2009-12-01 18:50 UTC (permalink / raw) To: Carl Worth; +Cc: notmuch@notmuchmail org [-- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value 2009-12-01 18:50 ` Dirk-Jan C. Binnema @ 2009-12-02 3:01 ` Carl Worth 0 siblings, 0 replies; 4+ messages in thread From: Carl Worth @ 2009-12-02 3:01 UTC (permalink / raw) To: djcb; +Cc: notmuch@notmuchmail org [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-12-02 3:01 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-11-23 6:21 [PATCH 2/2] * avoid gcc 4.4.1 compiler warning due to ignored 'fflush' return value Dirk-Jan C. Binnema 2009-12-01 16:08 ` Carl Worth 2009-12-01 18:50 ` Dirk-Jan C. Binnema 2009-12-02 3:01 ` Carl Worth
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).