Fix a memory leak in lpr

[trix_juniper.net]

Summary

chkprintcap/chkprintcap.c main
Free pcap_fname.

[pfg]

Add gad@ as he is the maintainer.
For the record: Coverity doesn't report any of these.

[ngie]

That's not the only fun... I'd probably incorporate this patch as well, but commit the calloc change separately:

$ svn diff usr.sbin/lpr/chkprintcap/
Index: usr.sbin/lpr/chkprintcap/chkprintcap.c
===================================================================
--- usr.sbin/lpr/chkprintcap/chkprintcap.c     (revision 314942)
+++ usr.sbin/lpr/chkprintcap/chkprintcap.c     (working copy)
@@ -113,6 +113,9 @@
         * the printcap file.
        */
        skres = skim_printcap(pcap_fname, verbosity);
+       if (skres == NULL)
+               return (1);
+
        if (skres->fatalerr)
                return (skres->fatalerr);

Index: usr.sbin/lpr/chkprintcap/skimprintcap.c
===================================================================
--- usr.sbin/lpr/chkprintcap/skimprintcap.c    (revision 314942)
+++ usr.sbin/lpr/chkprintcap/skimprintcap.c    (working copy)
@@ -81,8 +81,9 @@
        enum {NO_CONTINUE, WILL_CONTINUE, BAD_CONTINUE} is_cont, had_cont;
        enum {CMNT_LINE, ENTRY_LINE, TAB_LINE, TABERR_LINE} is_type, had_type;

-       skinf = malloc(sizeof(struct skiminfo));
-       memset(skinf, 0, sizeof(struct skiminfo));
+       skinf = calloc(1, sizeof(struct skiminfo));
+       if (skinf == NULL)
+               return (NULL);

        pc_file = fopen(pcap_fname, "r");
        if (pc_file == NULL) {

[ngie]

usr.sbin/lpr/chkprintcap/chkprintcap.c

161

This gets freed right before we exit main -- that's probably why the leak isn't noted.

[pfg]

In D9954#205869, @ngie wrote:

That's not the only fun... I'd probably incorporate this patch as well, but commit the calloc change separately:

$ svn diff usr.sbin/lpr/chkprintcap/
Index: usr.sbin/lpr/chkprintcap/chkprintcap.c
===================================================================
--- usr.sbin/lpr/chkprintcap/skimprintcap.c    (revision 314942)
+++ usr.sbin/lpr/chkprintcap/skimprintcap.c    (working copy)
@@ -81,8 +81,9 @@
        enum {NO_CONTINUE, WILL_CONTINUE, BAD_CONTINUE} is_cont, had_cont;
        enum {CMNT_LINE, ENTRY_LINE, TAB_LINE, TABERR_LINE} is_type, had_type;

-       skinf = malloc(sizeof(struct skiminfo));
-       memset(skinf, 0, sizeof(struct skiminfo));
+       skinf = calloc(1, sizeof(struct skiminfo));
+       if (skinf == NULL)
+               return (NULL);

        pc_file = fopen(pcap_fname, "r");
        if (pc_file == NULL) {

I am agnostic, but when malloc() has no multiplication some people (like bde) prefer to leave it untouched: you still have to add the NULL check right before the memset().

[gad]

[ thanks for contacting me ]

The change to free(pcap_fname) looks fine to me.

I wouldn't want to bother with change to use calloc() instead of malloc(), although it's obviously good to add the check for (skinf == NULL) after the malloc(), and once that is done then chkprintcap.c needs to check for (skres == NULL). I think I'd want to write the checking of skres a little differently though. I haven't done much on reviews.freebsd before, so let's see if I can upload an example patch (which I have not tested):

chkprint-test.diff762 BDownload

[gad]

Hmm. I guess that wasn't the best way to upload an example diff...

[gad]

Well, I tried to upload my diff as an "update", and that showed up as D9982 .

[gad]

I'm in the path of the big snowstorm which is supposed to hit tomorrow, so I might not respond to this for a few days. But once we've recovered from that, I'd be willing to commit my other patch for this along with the fix to skimprintcap.c. If nothing else, it shows that I'm still alive and paying (some) attention to lpr & friends.

[gad]

Accepting this, so I can later close it.

[gad]

This was handled by the commit which was referenced earlier.