On Thu, Nov 10, 2022 at 09:35:18AM +0100, Jeremie Courreges-Anglas wrote:
> On Thu, Nov 10 2022, Ross L Richardson <open...@rlr.id.au> wrote:
> > Reported upstream (by me) as
> > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=267684
> >
> > math/ministat has a silly bug in which the code assumes that "-" will be
> > specified no more than once at invocation:
> >
> > $ jot 3 | ministat - -
> > Segmentation fault (core dumped)
> >
> > The problem is in the port-patched code at:
> > 643 if (argc > (MAX_DS - 1))
> > 644 usage("Too many datasets.");
> > 645 nds = argc;
> > 646 for (i = 0; i < nds; i++) {
> > 647 setfilenames[i] = argv[i];
> > 648 if (!strcmp(argv[i], "-"))
> > 649 setfiles[0] = stdin;
> > 650 else
> > 651 setfiles[i] = fopen(argv[i],
> > "r");
> > 652 if (setfiles[i] == NULL)
> > 653 err(2, "Cannot open %s",
> > argv[i]);
> > 654 }
> >
> > On line 649, the index is fixed at 0, eventually leading to fgets()
> > attempting to read from an uninitialised stream.
> >
> > The simplest fix is change the index:
> > 649 setfiles[i] = stdin;
>
> Indeed.
>
> > That way, ministat will error out complaining that, on the second reading,
> > stdin has fewer than 3 data points.
> > (A more logical fix would be to check explicitly for more than 1
> > occurrence of "-".)
>
> A lot of tools that can use stdin don't explicitely check for it being
> specified twice. As far as this port is concerned, I think it's fine.
>
> Thanks for your report. Do you want to take it to upstream FreeBSD?
>
> Here's the diff for our ports tree.
FreeBSD version doesn't crash:
[freefall ~] ministat - -
ministat: Cannot open -: No error: 0
It's because on line 652 (see code snippet above) we check if array
element is set to NULL, and before that we either use 0 index for stdin
or don't set any element if we see any more -), so, I'd suggest to fail
the same way as FreeBSD does, for this we need to explicitly set all
array elements to NULL.
So, how about such approach?
idea:~$ ministat - -
ministat: Cannot open -: Undefined error: 0
diff /usr/ports
commit - 3dc099815c186e4b5eef9648d2e4013c689addbd
path + /usr/ports
blob - 757012eb733c5369eb84fd5f9f9c7802b03ff003
file + math/ministat/Makefile
--- math/ministat/Makefile
+++ math/ministat/Makefile
@@ -1,6 +1,7 @@
COMMENT= statistics utility
DISTNAME= ministat-0.0.20211218
+REVISION= 0
CATEGORIES= math
blob - 82421e27351ffc2918e2d7791636ad3c709d76f6
file + math/ministat/patches/patch-ministat_c
--- math/ministat/patches/patch-ministat_c
+++ math/ministat/patches/patch-ministat_c
@@ -1,5 +1,8 @@
Remove FBSDID and replace capsicum with pledge
+Safety belt for 'ministat - -' invocation see FreeBSD bugzilla
+https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=267684
+
Index: ministat.c
--- ministat.c.orig
+++ ministat.c
@@ -19,17 +22,21 @@ Index: ministat.c
#include <ctype.h>
#include <err.h>
#include <errno.h>
-@@ -569,6 +566,9 @@ main(int argc, char **argv)
+@@ -569,6 +566,13 @@ main(int argc, char **argv)
int termwidth = 74;
int suppress_plot = 0;
++ /* Safety belt for 'ministat - -' invocation */
++ for (i = 0; i < MAX_DS; i++)
++ setfiles[i] = NULL;
++
+ if (pledge("stdio rpath tty", NULL) == -1)
+ err(1, NULL);
+
if (isatty(STDOUT_FILENO)) {
struct winsize wsz;
-@@ -579,6 +579,9 @@ main(int argc, char **argv)
+@@ -579,6 +583,9 @@ main(int argc, char **argv)
termwidth = wsz.ws_col - 2;
}
@@ -39,7 +46,7 @@ Index: ministat.c
ci = -1;
while ((c = getopt(argc, argv, "AC:c:d:snqw:")) != -1)
switch (c) {
-@@ -651,23 +654,14 @@ main(int argc, char **argv)
+@@ -651,23 +658,14 @@ main(int argc, char **argv)
}
}
