On Thu, Dec 27, 2012 at 02:04:24PM +0100, Maxime Villard wrote:
> Well, as no one seems to give a fuck on tech@, I put a more
> glamourous title here.
>
> btw, i wonder why you don't put -Wextra to the makefile, you would
> see that there are a lot of unused parameters, comparisons between
> signed and unsigned, uninitialized vars, ...
Too many false positives to be useful.
People are running various static analysis packages over the tree
periodically to find such things. None are perfect so if you actually
find any you think are bugs, diffs are always appreciated.
.... Ken
>
>
> -------- Message original --------
> Sujet: [PATCH] pfctl: leak & stuff
> Date : Sat, 22 Dec 2012 08:16:09 +0100
> De : Maxime Villard <rusty...@gmx.fr>
> Pour : t...@openbsd.org
>
> Hi,
> here are my small changes for pfctl.
>
> 1) There are cases where we could leak a file descriptor by
> returning.
>
> 2) We don't need to check memory before freeing it, as
> free() already does that.
>
> 3) Just replaced a snprintf() by strlcpy(), it's faster.
>
> Ok/Comments ?
>
>
>
> Index: pfctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
> retrieving revision 1.314
> diff -u -r1.314 pfctl.c
> --- pfctl.c 19 Sep 2012 15:52:17 -0000 1.314
> +++ pfctl.c 22 Dec 2012 07:08:28 -0000
> @@ -1377,8 +1377,7 @@
> err(1, "DIOCXROLLBACK");
> exit(1);
> } else { /* sub ruleset */
> - if (path)
> - free(path);
> + free(path);
> return (-1);
> }
>
> @@ -1867,10 +1866,6 @@
> unsigned int len = 0;
> size_t n;
>
> - f = fopen(file, "w");
> - if (f == NULL)
> - err(1, "open: %s", file);
> -
> memset(&ps, 0, sizeof(ps));
> for (;;) {
> ps.ps_len = len;
> @@ -1893,6 +1888,10 @@
> return; /* no states */
> len *= 2;
> }
> +
> + f = fopen(file, "w");
> + if (f == NULL)
> + err(1, "open: %s", file);
>
> n = ps.ps_len / sizeof(struct pfsync_state);
> if (fwrite(inbuf, sizeof(struct pfsync_state), n, f) < n)
> Index: pfctl_osfp.c
> ===================================================================
> RCS file: /cvs/src/sbin/pfctl/pfctl_osfp.c,v
> retrieving revision 1.18
> diff -u -r1.18 pfctl_osfp.c
> --- pfctl_osfp.c 18 Oct 2010 15:55:28 -0000 1.18
> +++ pfctl_osfp.c 22 Dec 2012 07:08:28 -0000
> @@ -112,16 +112,11 @@
>
> while ((line = fgetln(in, &len)) != NULL) {
> lineno++;
> - if (class)
> - free(class);
> - if (version)
> - free(version);
> - if (subtype)
> - free(subtype);
> - if (desc)
> - free(desc);
> - if (tcpopts)
> - free(tcpopts);
> + free(class);
> + free(version);
> + free(subtype);
> + free(desc);
> + free(tcpopts);
> class = version = subtype = desc = tcpopts = NULL;
> memset(&fp, 0, sizeof(fp));
>
> @@ -250,16 +245,11 @@
> add_fingerprint(dev, opts, &fp);
> }
>
> - if (class)
> - free(class);
> - if (version)
> - free(version);
> - if (subtype)
> - free(subtype);
> - if (desc)
> - free(desc);
> - if (tcpopts)
> - free(tcpopts);
> + free(class);
> + free(version);
> + free(subtype);
> + free(desc);
> + free(tcpopts);
>
> fclose(in);
>
> @@ -513,7 +503,7 @@
> return (buf);
>
> found:
> - snprintf(buf, len, "%s", class_name);
> + strlcpy(buf, class_name, len);
> if (version_name) {
> strlcat(buf, " ", len);
> strlcat(buf, version_name, len);
> Index: pfctl_radix.c
> ===================================================================
> RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v
> retrieving revision 1.29
> diff -u -r1.29 pfctl_radix.c
> --- pfctl_radix.c 27 Jul 2011 00:26:10 -0000 1.29
> +++ pfctl_radix.c 22 Dec 2012 07:08:28 -0000
> @@ -499,8 +499,7 @@
> {
> if (b == NULL)
> return;
> - if (b->pfrb_caddr != NULL)
> - free(b->pfrb_caddr);
> + free(b->pfrb_caddr);
> b->pfrb_caddr = NULL;
> b->pfrb_size = b->pfrb_msize = 0;
> }
