[patch] fix a few little buglets in mutt HEAD
I have access to a static analysis tool. And I happen to
be a long time user of mutt. I would like to continue to be a user.
So I ran the tool over HEAD to see what it found. It found a lot. But
here are a few things that I am pretty sure could be fixed with little
impact.
E
--
Erik Hovland
erik@xxxxxxxxxxx
http://hovland.org/
1. len should be a signed type.
len stores the return value of functions that return signed values.
So it should be signed. Changing to ssize_t.
2. i will never be -1
Because of the organization of the parens at line 1985, i will be
assigned the evaulation of stat() == -1. Which is either 0 or 1.
Later on i is checked for -1. So that means that the caller likely
intended i to be assigned the return value of stat() instead.
3. ap_retry might never be ended if the condition is never executed.
At least on x86 va_end is a no-op. But on platforms like PowerPC it
is a requirement that if any va_list is started, it needs to be
ended. In the function mutt_buffer_printf ap_retry might not be
ended if the conditional never evaluates true. To prevent the va_list
never ending we can move the va_end() out of the conditional.
4. Since getc returns int, the variable that stores it should be that type.
If the variable that stores the return value of getc is smaller then
int (like a char in this case) then the return value is truncated.
5. At the point that this conditional fires fp can only be NULL.
fp can only be NULL. Implying that this is not really the file
pointer that the caller wants to close. Since the unlink is for the
temporary file the fix is to use the file pointer s.fpout, which does
point to that file.
6. buflen cannot be zero at this check. Remove it.
7. Checking null on stack allocated string has no effect
In smime_get_field_from_db() at line 533 there is a check
to see if fields[4] is null. But the strings of fields are
stack allocated at lone 487. So the check will always be
false. Better to remove the check.
8. h is unsigned. Don't check for less then zero.
In hash_string() h is unsigned. So checking less then zero has no effect
at all on the outcome. Better to remove the code there altogether.
9. fread returns 0 or more
mutt_create_alias() makes a call to fread at line 359. It compares the
return of fread to les then zero. But fread can only return 0 or more.
The fix is to have it check to see if it read one unit (in this case a
byte) and error out if it didn't.
diff --git a/bcache.c b/bcache.c
--- a/bcache.c
+++ b/bcache.c
@@ -43,7 +43,7 @@ static int bcache_path(ACCOUNT *account,
{
char host[STRING];
ciss_url_t url;
- size_t len;
+ ssize_t len;
if (!account || !MessageCachedir || !*MessageCachedir || !dst || !dstlen)
return -1;
diff --git a/mh.c b/mh.c
--- a/mh.c
+++ b/mh.c
@@ -1982,7 +1982,7 @@ int mh_check_mailbox (CONTEXT * ctx, int
/* create .mh_sequences when there isn't one. */
snprintf (buf, sizeof (buf), "%s/.mh_sequences", ctx->path);
- if ((i = stat (buf, &st_cur) == -1) && errno == ENOENT)
+ if ((i = stat (buf, &st_cur)) == -1 && errno == ENOENT)
{
char *tmp;
FILE *fp = NULL;
diff --git a/muttlib.c b/muttlib.c
--- a/muttlib.c
+++ b/muttlib.c
@@ -1624,11 +1624,11 @@ int mutt_buffer_printf (BUFFER* buf, con
safe_realloc (&buf->data, buf->dsize);
buf->dptr = buf->data + doff;
len = vsnprintf (buf->dptr, len, fmt, ap_retry);
- va_end (ap_retry);
}
if (len > 0)
buf->dptr += len;
+ va_end (ap_retry);
va_end (ap);
return len;
diff --git a/parse.c b/parse.c
--- a/parse.c
+++ b/parse.c
@@ -41,7 +41,7 @@ char *mutt_read_rfc822_line (FILE *f, ch
char *mutt_read_rfc822_line (FILE *f, char *line, size_t *linelen)
{
char *buf = line;
- char ch;
+ int ch;
size_t offset = 0;
FOREVER
diff --git a/pattern.c b/pattern.c
--- a/pattern.c
+++ b/pattern.c
@@ -180,9 +180,9 @@ msg_search (CONTEXT *ctx, pattern_t* pat
&& !crypt_valid_passphrase(h->security))
{
mx_close_message (&msg);
- if (fp)
+ if (s.fpout)
{
- fclose (fp);
+ fclose (s.fpout);
unlink (tempfile);
}
return (0);
diff --git a/rfc822.c b/rfc822.c
--- a/rfc822.c
+++ b/rfc822.c
@@ -594,8 +594,6 @@ void rfc822_write_address_single (char *
{
if (*pc == '"' || *pc == '\\')
{
- if (!buflen)
- goto done;
*pbuf++ = '\\';
buflen--;
}
diff --git a/smime.c b/smime.c
--- a/smime.c
+++ b/smime.c
@@ -530,8 +530,7 @@ char *smime_get_field_from_db (char *mai
if (numFields < 2)
continue;
if (mailbox && public &&
- (!fields[4] ||
- *fields[4] == 'i' || *fields[4] == 'e' || *fields[4] == 'r'))
+ (*fields[4] == 'i' || *fields[4] == 'e' || *fields[4] == 'r'))
continue;
if (found)
diff --git a/hash.c b/hash.c
--- a/hash.c
+++ b/hash.c
@@ -39,7 +39,6 @@ int hash_string (const unsigned char *s,
while (*s)
h += (h << 7) + *s++;
h = (h * SOMEPRIME) % n;
- h = (h >= 0) ? h : h + n;
#endif
return (signed) (h % n);
diff --git a/alias.c b/alias.c
--- a/alias.c
+++ b/alias.c
@@ -356,7 +356,7 @@ retry_name:
{
if (fseek (rc, -1, SEEK_CUR) < 0)
goto fseek_err;
- if (fread(buf, 1, 1, rc) < 0)
+ if (fread(buf, 1, 1, rc) != 1)
{
mutt_perror (_("Error reading alias file"));
return;