At 10:51 -0500 22 Aug 2013, Derek Martin <[email protected]> wrote:
Given this code and the fix, it seems likely that the problem you ran into is that err.dptr is used uninitialized. Since err is half initialized in the code that follows, I personally think it would be slightly preferable to add:err.dptr = NULL; /* not sure if this is used, but that's not a reason to skip initializing it */ err.destroy = 0;
Yes, the first version of the fix that I tried I just set dptr to NULL, and that resolved the problem. But, I decided to go with the memset to be consistent with the initialization of the token buffer that occurs right after this portion of the code.
It's slighly more code, but it's clearer and avoids assigning err.dsize and err.data twice.
It definitely avoids assigning to those elements twice, but which is clearer is arguable. Using memset makes it clear that the entire structure is being initialized to known values, setting individual elements could leave readers questioning what is happening with other elements. Using memset also has the advantage that it would automatically clear elements that would be added to the structure in the future.
But that said, your fix seems fine. THAT said, I'm betting there are other places in the code where there are BUFFER structs which aren't initialized properly. Might be worth creating a function/macro to initialize these things properly... e.g.:
There's actually already a mutt_buffer_init function which is just a wrapper around memset, but it's only called in one place.
And I did find a number of other places where buffers aren't properly initialized. The end of this message is a patch which uses mutt_buffer_init to initialize those, as well as the one fixed by my earlier patch (which should be discarded).
I'll followup with another patch to use that function for buffer initializations which are currently being done with memset.
initialize_buffer(BUFFER *b, size_t size, int destroy)
{
b->dsize = size;
b->data = SOME_MALLOC(size);
b->dptr = NULL;
b->destroy = destroy;
}
There are a number of places where the data for the buffer is allocated in a different place than the buffer itself, or where the buffer data is a stack variable. That type of API wouldn't work in those cases.
------- 8< -----------
Subject: [PATCH] Initialize BUFFER variables
Ensure that BUFFER variables are initialized to prevent later attempts
to traverse an uninitialized pointer.
---
commands.c | 1 +
hook.c | 7 +++++--
imap/command.c | 1 +
imap/imap.c | 1 +
4 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/commands.c b/commands.c
index 13b12dd..357b354 100644
--- a/commands.c
+++ b/commands.c
@@ -618,6 +618,7 @@ void mutt_enter_command (void)
buffer[0] = 0;
if (mutt_get_field (":", buffer, sizeof (buffer), M_COMMAND) != 0 ||
!buffer[0])
return;
+ mutt_buffer_init (&err);
err.dsize = STRING;
err.data = safe_malloc(err.dsize);
memset (&token, 0, sizeof (token));
diff --git a/hook.c b/hook.c
index 3fdcfb2..f9f8588 100644
--- a/hook.c
+++ b/hook.c
@@ -281,7 +281,8 @@ void mutt_folder_hook (char *path)
BUFFER err, token;
current_hook_type = M_FOLDERHOOK;
-
+
+ mutt_buffer_init (&err); err.dsize = STRING; err.data = safe_malloc (err.dsize); memset (&token, 0, sizeof (token)); @@ -332,7 +333,8 @@ void mutt_message_hook (CONTEXT *ctx, HEADER *hdr, int type) HOOK *hook; current_hook_type = type;- +
+ mutt_buffer_init (&err);
err.dsize = STRING;
err.data = safe_malloc (err.dsize);
memset (&token, 0, sizeof (token));
@@ -476,6 +478,7 @@ void mutt_account_hook (const char* url)
if (inhook)
return;
+ mutt_buffer_init (&err);
err.dsize = STRING;
err.data = safe_malloc (err.dsize);
memset (&token, 0, sizeof (token));
diff --git a/imap/command.c b/imap/command.c
index 4b47de2..6dfeb62 100644
--- a/imap/command.c
+++ b/imap/command.c
@@ -778,6 +778,7 @@ static void cmd_parse_lsub (IMAP_DATA* idata, char* s)
url_ciss_tostring (&url, buf + 11, sizeof (buf) - 10, 0);
safe_strcat (buf, sizeof (buf), "\"");
memset (&token, 0, sizeof (token));
+ mutt_buffer_init (&err);
err.data = errstr;
err.dsize = sizeof (errstr);
if (mutt_parse_rc_line (buf, &token, &err))
diff --git a/imap/imap.c b/imap/imap.c
index 83b05d6..b263abf 100644
--- a/imap/imap.c
+++ b/imap/imap.c
@@ -1828,6 +1828,7 @@ int imap_subscribe (char *path, int subscribe)
if (option (OPTIMAPCHECKSUBSCRIBED))
{
memset (&token, 0, sizeof (token));
+ mutt_buffer_init (&err);
err.data = errstr;
err.dsize = sizeof (errstr);
snprintf (mbox, sizeof (mbox), "%smailboxes \"%s\"",
--
1.7.10.4
pgpD99kalZKF4.pgp
Description: PGP signature
