nmh-workers
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Nmh-workers] [patch] gcc warnings: possible use of uninitialized da


From: Peter Maydell
Subject: Re: [Nmh-workers] [patch] gcc warnings: possible use of uninitialized data
Date: Fri, 05 Nov 2010 22:41:28 +0000

markus schnalke wrote:
[gcc warning fixes]

I agree that these should be dealt with (so as a developer you
can eventually choose to compile with -Werror); however I think
that in at least some cases there's actually an underlying issue
that should be dealt with rather than just silencing the
warning. See my comments below.

(There's also two warnings from compiling the output of
flex, which are irritating because they can be silenced
but only by adding a flex-specific "%options noinput nounput"
which we can't do because it would break portability.
Adding autoconfery to detect flex and add the option line
seems like way overkill. Perhaps we should just special
case the lex output and not pass it the warning options.)


diff -r 7e963436013a mts/smtp/smtp.c
>--- a/mts/smtp/smtp.c  Fri Nov 05 10:56:54 2010 -0300
>+++ b/mts/smtp/smtp.c  Fri Nov 05 16:36:28 2010 -0300
>@@ -1208,7 +1208,7 @@
> static int
> smhear (void)
> {
>-    int i, code, cont, bc, rc, more;
>+    int i, code, cont, bc = 0, rc, more;
>     unsigned char *bp;
>     char *rp;
>     char **ehlo = NULL, buffer[BUFSIZ];
>@@ -1326,6 +1326,9 @@
> }

Agreed; applied.

>diff -r 7e963436013a sbr/fmt_rfc2047.c
>--- a/sbr/fmt_rfc2047.c        Fri Nov 05 10:56:54 2010 -0300
>+++ b/sbr/fmt_rfc2047.c        Fri Nov 05 16:36:28 2010 -0300
>@@ -239,6 +239,7 @@
>               /* base64 */
>               int c1, c2, c3, c4;
> 
>+              c1 = c2 = c3 = c4 = 0;
>               pp = startofmime;
>               while (pp < endofmime) {
>                   /* 6 + 2 bits */

Maybe these should be -1? Maybe this code for de-base64'ing
could be replaced with something less cryptic instead :-)


>diff -r 7e963436013a sbr/folder_addmsg.c
>--- a/sbr/folder_addmsg.c      Fri Nov 05 10:56:54 2010 -0300
>+++ b/sbr/folder_addmsg.c      Fri Nov 05 16:36:28 2010 -0300
>@@ -22,7 +22,7 @@
> folder_addmsg (struct msgs **mpp, char *msgfile, int selected,
>                int unseen, int preserve, int deleting, char *from_dir)
> {
>-    int infd, outfd, linkerr, first_time, msgnum;
>+    int infd, outfd, linkerr, first_time, msgnum = 0;
>     char *nmsg, newmsg[BUFSIZ];
>     char oldmsg[BUFSIZ];
>     struct msgs *mp;

This one might be better addressed by the following patch, which
makes the code a bit easier to understand for humans and
compilers both; although I haven't tested it beyond confirming
that it does silence the warning :-)

===begin===
Index: sbr/folder_addmsg.c
===================================================================
RCS file: /cvsroot/nmh/nmh/sbr/folder_addmsg.c,v
retrieving revision 1.8
diff -u -r1.8 folder_addmsg.c
--- sbr/folder_addmsg.c 21 Mar 2007 00:21:10 -0000      1.8
+++ sbr/folder_addmsg.c 5 Nov 2010 22:03:42 -0000
@@ -22,39 +22,30 @@
 folder_addmsg (struct msgs **mpp, char *msgfile, int selected,
                int unseen, int preserve, int deleting, char *from_dir)
 {
-    int infd, outfd, linkerr, first_time, msgnum;
+    int infd, outfd, linkerr, msgnum;
     char *nmsg, newmsg[BUFSIZ];
     char oldmsg[BUFSIZ];
     struct msgs *mp;
     struct stat st1, st2;
 
-    first_time = 1;    /* this is first attempt */
     mp = *mpp;
 
+    /* should we preserve the numbering of the message? */
+    if (preserve && (msgnum = m_atoi (msgfile)) > 0) {
+       ;
+    } else if (mp->nummsg == 0) {
+       /* check if we are adding to empty folder */
+       msgnum = 1;
+    } else {
+       /* else use highest message number + 1 */
+       msgnum = mp->hghmsg + 1;
+    }
+     
     /*
      * We might need to make several attempts
      * in order to add the message to the folder.
      */
-    for (;;) {
-       /*
-        * Get the message number we will attempt to add.
-        */
-       if (first_time) {
-           /* should we preserve the numbering of the message? */
-           if (preserve && (msgnum = m_atoi (msgfile)) > 0) {
-               ;
-           } else if (mp->nummsg == 0) {
-               /* check if we are adding to empty folder */
-               msgnum = 1;
-           } else {
-               /* else use highest message number + 1 */
-               msgnum = mp->hghmsg + 1;
-           }
-           first_time = 0;
-       } else {
-           /* another attempt, so try next higher message number */
-           msgnum++;
-       }
+    for (;; msgnum++) {
 
        /*
         * See if we need more space.  If we need space at the
===endit===


>diff -r 7e963436013a uip/inc.c
>--- a/uip/inc.c        Fri Nov 05 10:56:54 2010 -0300
>+++ b/uip/inc.c        Fri Nov 05 16:36:28 2010 -0300
>@@ -236,7 +236,7 @@
> {
>     int chgflag = 1, trnflag = 1;
>     int noisy = 1, width = 0;
>-    int rpop, i, hghnum = 0, msgnum = 0;
>+    int rpop, i = 0, hghnum = 0, msgnum = 0;
>     int kpop = 0, sasl = 0;
>     char *cp, *maildir = NULL, *folder = NULL;
>     char *format = NULL, *form = NULL;

NAK. This seems to be a rather large function which is using
'i' as a "did we hit an error?" flag to see whether we should
take an error-exit path at the end. However (a) it is very
poorly named for this purpose (something like 'err' would
be better) and (b) some POP code in the middle has borrowed
it for a loop index variable, probably because of the awful
name, resulting in this disgusting ifdef:

#ifdef POP
    if (p < 0) {                /* error */
#else
    if (i < 0) {                /* error */
#endif

We should clean this up.

>diff -r 7e963436013a uip/new.c
>--- a/uip/new.c        Fri Nov 05 10:56:54 2010 -0300
>+++ b/uip/new.c        Fri Nov 05 16:36:28 2010 -0300
>@@ -297,7 +297,7 @@
> static struct node *
> doit(char *cur, char *folders, char *sequences[])
> {
>-    struct node *first, *cur_node, *node, *last, *prev;
>+    struct node *first, *cur_node, *node, *last = NULL, *prev;
>     size_t folder_len;
>     int count, total = 0;
>     char *command = NULL, *sequences_s = NULL;

I think this one may be indicating a genuine bug where
we don't do the right thing if check_folders() sets
first to NULL. We should fix the bug instead.

-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]