Page MenuHomeFreeBSD

Fix a memory leak in lpr
ClosedPublic

Authored by trix_juniper.net on Mar 11 2017, 1:53 AM.

Details

Summary

chkprintcap/chkprintcap.c main
Free pcap_fname.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pfg added a reviewer: gad.Mar 11 2017, 4:03 AM
pfg edited edge metadata.

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

ngie edited edge metadata.Mar 12 2017, 1:39 AM

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 added inline comments.Mar 12 2017, 1:43 AM
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 added a comment.Mar 12 2017, 2:07 AM
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 edited edge metadata.Mar 12 2017, 9:18 PM

[ 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):

gad added a comment.Mar 12 2017, 9:19 PM

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

gad added a comment.Mar 12 2017, 9:36 PM

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

gad added a comment.Mar 14 2017, 1:14 AM

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 accepted this revision.Mar 23 2017, 3:07 AM

Accepting this, so I can later close it.

This revision is now accepted and ready to land.Mar 23 2017, 3:07 AM
gad closed this revision.Mar 23 2017, 3:08 AM

This was handled by the commit which was referenced earlier.