Re: [Cvs-dev] Re: Result of CVS Coverity scan, via NetBSD

[Mark D. Baushke]

Hi Christos,

Christos Zoulas <address@hidden> writes:

> On May 12,  6:35am, address@hidden ("Mark D. Baushke") wrote:
> -- Subject: Re: [Cvs-dev] Re: Result of CVS Coverity scan, via NetBSD
> 
> | Would it make sense to modify src/gnu/usr.bin/cvs/include/config.h
> | so that it does a
> | 
> | /* Functions defined in src/gnu/dist/xcvs/src/subr.c */
> | #define xasprintf cvs_xasprintf
> | #define xmalloc   cvs_xmalloc
> | #define xrealloc  cvs_xrealloc
> | #define xstrdup   cvs_xstrdup
> | 
> | or, does the coverity model processing not handle macro processing
> | correctly?
> 
> It does, and it will work properly [provided it compiles :-)]

You might give it a try and see if it works for you.

> | For what it is worth, I have not finished going through all of the run
> | 22 reported problems. I know that there is at least a few more that are
> | 'real' bugs (cid-1058 and cid-1061 are both such).
> | 
> | Given that our xstrdup() is safe, I will probably be considering reverting
> | the=20
> | 
> |     var = str ? xstrdup (str) : NULL;
> | 
> | changes back into
> | 
> |     var = xstrdup (str);
> 
> Yes, we should revert the changes.

Agreed. The patch below reverts those changes and adds a few asserts to
deal with the other cases. I have committed it to the CVS STABLE branch
(1.11.x).

> | in the CVS STABLE and CVS FEATURE source bases. (It turns out that CVS
> | FEATURE uses Xstrdup to do the NULL check before calling the real
> | xstrdup () function, but has a '#define xstrdup Xstrdup' to hide it.)
> 
> Ouch, pre-processor macro hell...

Indeed.

        -- Mark

Index: ChangeLog
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/ChangeLog,v
retrieving revision 1.2
diff -u -p -r1.2 ChangeLog
--- ChangeLog   4 May 2006 15:39:34 -0000       1.2
+++ ChangeLog   12 May 2006 14:28:53 -0000
@@ -1,18 +1,67 @@
+2006-05-12  Mark D. Baushke  <address@hidden>
+
+       * rcs.c (RCS_isdead): Assert that the first argument is not NULL.
+       [Fixes NetBSD cid-1058.]
+
+       * commit.c (checkaddfile): Do not dereference NULL on call to
+       error().
+       [Fixes NetBSD cid-1061.]
+
+       * log.c (cvslog): Assert p->start && p->end instead of masking the
+       problem.
+       * server.c (server_updated): Assert findnode_fn results instead of
+       masking the problem.
+       
+       * add.c (add_directory): Revert previous change. The xstrdup()
+       function already deals a NULL argument.
+       * client.c (handle_mt): Ditto.
+       * entries.c (Entnode_Create): Ditto.
+       (Entries_Open): Ditto.
+       * logmsg.c (fmt_proc): Ditto.
+       * vers_ts.c (Version_TS): Ditto.
+       
+2006-05-11  Mark D. Baushke  <address@hidden>
+
+       * add.c (add_directory): Protect tag from NULL dereference.
+       [Fixes NetBSD cid-1054.]
+
+       * client.c (handle_mt): Deal with missing text argument.
+       [Fixes NetBSD cid-924.]
+
+       * entries.c (Entnode_Create): Protect date, tag and ts_conflict
+       from possible NULL dereference.
+       [Fixes NetBSD coverity cid-994, cid-995, cid-1055, cid-1057.]
+
+       * entries.c (Entries_Open): Protect dirtag and dirdate from
+       possible NULL dereference.
+       [Fixes NetBSD coverity cid-996.]
+
+       * log.c (cvslog): Validate start and end args to
+       date_to_internet().
+       [Fixes NetBSD coverity cid-2427 and cid-2428.]
+
+       * logmsg.c (fmt_proc): Protect li->tag from NULL dereference.
+       [Fixes NetBSD coverity cid-997.]
+
+       * vers_ts.c (Version_TS): Protect tag and vers_ts->tag from NULL
+       dereference.
+       [Fixes NetBSD coverity cid-1053.]
+
 2006-05-04  Mark D. Baushke  <address@hidden>
 
-       * filesubr.c (cvs_temp_file): Avoid keeping pointers to free()'d
-       storage laying around.
-       * commit.c (commit): Handle possible NULL filename values
-       returned from cvs_temp_file().
-       * filesubr.c (cvs_temp_name): Ditto.
-       * import.c (import): Ditto.
-       * login.c (password_entry_operation): Ditto.
-       * logmsg.c (do_verify): Ditto.
-       * patch.c (patch_fileproc): Ditto.
-       [Fixes NetBSD coverity cid-2545.]
+       * filesubr.c (cvs_temp_file): Avoid keeping pointers to free()'d
+       storage laying around.
+       * commit.c (commit): Handle possible NULL filename values
+       returned from cvs_temp_file().
+       * filesubr.c (cvs_temp_name): Ditto.
+       * import.c (import): Ditto.
+       * login.c (password_entry_operation): Ditto.
+       * logmsg.c (do_verify): Ditto.
+       * patch.c (patch_fileproc): Ditto.
+       [Fixes NetBSD coverity cid-2545.]
 
-       * buffer.c (packetizing_buffer_output): Initialize outdata.
-       [Fixes NetBSD coverity cid-2474.]
+       * buffer.c (packetizing_buffer_output): Initialize outdata.
+       [Fixes NetBSD coverity cid-2474.]
 
        * server.c (server_updated): Fix NetBSD coverity cid-1352
        NetBSD-sparc64 of 2006-May-02 03:02:46.
Index: add.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/add.c,v
retrieving revision 1.2
diff -u -p -r1.2 add.c
--- add.c       11 May 2006 17:47:49 -0000      1.2
+++ add.c       12 May 2006 14:28:53 -0000
@@ -852,7 +852,7 @@ add_directory (finfo)
        p->key = xstrdup ("- New directory");
        li = (struct logfile_info *) xmalloc (sizeof (struct logfile_info));
        li->type = T_TITLE;
-       li->tag = tag ? xstrdup (tag) : NULL;
+       li->tag = xstrdup (tag);
        li->rev_old = li->rev_new = NULL;
        p->data = li;
        (void) addnode (ulist, p);
Index: client.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/client.c,v
retrieving revision 1.4
diff -u -p -r1.4 client.c
--- client.c    11 May 2006 17:47:49 -0000      1.4
+++ client.c    12 May 2006 14:28:54 -0000
@@ -3277,7 +3277,7 @@ handle_mt (args, len)
                        cvs_output ("\n", 1);
                        free (updated_fname);
                    }
-                   updated_fname = text ? xstrdup (text) : NULL;
+                   updated_fname = xstrdup (text);
                }
                /* Swallow all other tags.  Either they are extraneous
                   or they reflect future extensions that we can
@@ -3288,11 +3288,11 @@ handle_mt (args, len)
                if (strcmp (tag, "conflicts") == 0)
                    importmergecmd.conflicts = text ? atoi (text) : -1;
                else if (strcmp (tag, "mergetag1") == 0)
-                   importmergecmd.mergetag1 = text ? xstrdup (text) : NULL;
+                   importmergecmd.mergetag1 = xstrdup (text);
                else if (strcmp (tag, "mergetag2") == 0)
-                   importmergecmd.mergetag2 = text ? xstrdup (text) : NULL;
+                   importmergecmd.mergetag2 = xstrdup (text);
                else if (strcmp (tag, "repository") == 0)
-                   importmergecmd.repository = text ? xstrdup (text) : NULL;
+                   importmergecmd.repository = xstrdup (text);
                /* Swallow all other tags.  Either they are text for
                    which we are going to print our own version when we
                    see -importmergecmd, or they are future extensions
Index: commit.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/commit.c,v
retrieving revision 1.3
diff -u -p -r1.3 commit.c
--- commit.c    4 May 2006 15:39:34 -0000       1.3
+++ commit.c    12 May 2006 14:28:54 -0000
@@ -2127,7 +2127,7 @@ checkaddfile (file, repository, tag, opt
            rcs = RCS_parse (file, repository);
            if (rcs == NULL)
            {
-               error (0, 0, "could not read %s", rcs->path);
+               error (0, 0, "could not read %s in %s", file, repository);
                goto out;
            }
            *rcsnode = rcs;
Index: entries.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/entries.c,v
retrieving revision 1.3
diff -u -p -r1.3 entries.c
--- entries.c   11 May 2006 17:47:49 -0000      1.3
+++ entries.c   12 May 2006 14:28:54 -0000
@@ -59,9 +59,9 @@ Entnode_Create(type, user, vn, ts, optio
     ent->version   = xstrdup (vn);
     ent->timestamp = xstrdup (ts ? ts : "");
     ent->options   = xstrdup (options ? options : "");
-    ent->tag       = tag ? xstrdup (tag) : NULL;
-    ent->date      = date ? xstrdup (date) : NULL;
-    ent->conflict  = ts_conflict ? xstrdup (ts_conflict) : NULL;
+    ent->tag       = xstrdup (tag);
+    ent->date      = xstrdup (date);
+    ent->conflict  = xstrdup (ts_conflict);
 
     return ent;
 }
@@ -491,8 +491,8 @@ Entries_Open (aflag, update_dir)
        sdtp = (struct stickydirtag *) xmalloc (sizeof (*sdtp));
        memset ((char *) sdtp, 0, sizeof (*sdtp));
        sdtp->aflag = aflag;
-       sdtp->tag = dirtag ? xstrdup (dirtag) : NULL;
-       sdtp->date = dirdate ? xstrdup (dirdate) : NULL;
+       sdtp->tag = xstrdup (dirtag);
+       sdtp->date = xstrdup (dirdate);
        sdtp->nonbranch = dirnonbranch;
 
        /* feed it into the list-private area */
Index: log.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/log.c,v
retrieving revision 1.2
diff -u -p -r1.2 log.c
--- log.c       11 May 2006 17:47:49 -0000      1.2
+++ log.c       12 May 2006 14:28:54 -0000
@@ -316,39 +316,33 @@ cvslog (argc, argv)
        {
            p = log_data.datelist;
            log_data.datelist = p->next;
-           if (p->start != NULL && p->end != NULL)
-           {
-               send_to_server ("Argument -d\012", 0);
-               send_to_server ("Argument ", 0);
-               date_to_internet (datetmp, p->start);
-               send_to_server (datetmp, 0);
-               if (p->inclusive)
-                   send_to_server ("<=", 0);
-               else
-                   send_to_server ("<", 0);
-               date_to_internet (datetmp, p->end);
-               send_to_server (datetmp, 0);
-               send_to_server ("\012", 0);
-           }
-           if (p->start)
-               free (p->start);
-           if (p->end)
-               free (p->end);
+           assert (p->start != NULL && p->end != NULL);
+           send_to_server ("Argument -d\012", 0);
+           send_to_server ("Argument ", 0);
+           date_to_internet (datetmp, p->start);
+           send_to_server (datetmp, 0);
+           if (p->inclusive)
+               send_to_server ("<=", 0);
+           else
+               send_to_server ("<", 0);
+           date_to_internet (datetmp, p->end);
+           send_to_server (datetmp, 0);
+           send_to_server ("\012", 0);
+           free (p->start);
+           free (p->end);
            free (p);
        }
        while (log_data.singledatelist != NULL)
        {
            p = log_data.singledatelist;
            log_data.singledatelist = p->next;
-           if (p->end)
-           {
-               send_to_server ("Argument -d\012", 0);
-               send_to_server ("Argument ", 0);
-               date_to_internet (datetmp, p->end);
-               send_to_server (datetmp, 0);
-               send_to_server ("\012", 0);
-               free (p->end);
-           }
+           assert (p->end != NULL);
+           send_to_server ("Argument -d\012", 0);
+           send_to_server ("Argument ", 0);
+           date_to_internet (datetmp, p->end);
+           send_to_server (datetmp, 0);
+           send_to_server ("\012", 0);
+           free (p->end);
            free (p);
        }
            
Index: logmsg.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/logmsg.c,v
retrieving revision 1.4
diff -u -p -r1.4 logmsg.c
--- logmsg.c    11 May 2006 17:47:49 -0000      1.4
+++ logmsg.c    12 May 2006 14:28:54 -0000
@@ -155,7 +155,7 @@ fmt_proc (p, closure)
 
            if (tag != NULL)
                free (tag);
-           tag = li->tag ? xstrdup (li->tag) : NULL;
+           tag = xstrdup (li->tag);
 
            /* Force a new line.  */
            col = 70;
Index: rcs.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/rcs.c,v
retrieving revision 1.2
diff -u -p -r1.2 rcs.c
--- rcs.c       4 Feb 2006 16:29:56 -0000       1.2
+++ rcs.c       12 May 2006 14:28:54 -0000
@@ -3487,6 +3487,8 @@ RCS_isdead (rcs, tag)
     Node *p;
     RCSVers *version;
 
+    assert (rcs != NULL);
+
     if (rcs->flags & PARTIAL)
        RCS_reparsercsfile (rcs, (FILE **) NULL, (struct rcsbuffer *) NULL);
 
Index: server.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/server.c,v
retrieving revision 1.4
diff -u -p -r1.4 server.c
--- server.c    4 May 2006 15:39:34 -0000       1.4
+++ server.c    12 May 2006 14:28:54 -0000
@@ -4235,6 +4235,7 @@ CVS server internal error: no mode in se
               in case we end up processing it again (e.g. modules3-6
               in the testsuite).  */
            node = findnode_fn (finfo->entries, finfo->file);
+           assert (node != NULL);
            if (node != NULL)
            {
                Entnode *entnode = node->data;
Index: vers_ts.c
===================================================================
RCS file: /cvsroot/src/gnu/dist/xcvs/src/vers_ts.c,v
retrieving revision 1.2
diff -u -p -r1.2 vers_ts.c
--- vers_ts.c   11 May 2006 17:47:49 -0000      1.2
+++ vers_ts.c   12 May 2006 14:28:54 -0000
@@ -155,8 +155,8 @@ Version_TS (finfo, options, tag, date, f
      */
     if (tag || date)
     {
-       vers_ts->tag = tag ? xstrdup (tag) : NULL;
-       vers_ts->date = date ? xstrdup (date) : NULL;
+       vers_ts->tag = xstrdup (tag);
+       vers_ts->date = xstrdup (date);
     }
     else if (!vers_ts->entdata && (sdtp && sdtp->aflag == 0))
     {
@@ -200,7 +200,7 @@ Version_TS (finfo, options, tag, date, f
            if (vers_ts->vn_rcs == NULL)
                vers_ts->vn_tag = NULL;
            else if (simple)
-               vers_ts->vn_tag = vers_ts->tag ? xstrdup (vers_ts->tag) : NULL;
+               vers_ts->vn_tag = xstrdup (vers_ts->tag);
            else
                vers_ts->vn_tag = xstrdup (vers_ts->vn_rcs);
        }