On Thu, Nov 10 2022, Mikhail <mp39...@gmail.com> wrote: > 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?
Setting the array to NULL prevents the crash - FreeBSD sems to be more forgiving here - but IMO the intended semantics are really to set setfiles[i]. I think that the proposal from Ross should be preferred, for the ports tree but also for an upstream fix. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
