On Sat, Nov 12 2022, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote: > On Thu, Nov 10 2022, Mikhail <mp39...@gmail.com> wrote: >> On Thu, Nov 10, 2022 at 06:21:52PM +0100, Jeremie Courreges-Anglas wrote: >>> 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. >> >> I created differential revision for Ross' patch: >> https://reviews.freebsd.org/D37346 > > Thanks! > >> If it won't be handled by FreeBSD in reasonable time, I think we can >> proceed with your patch without upstream then. > > I see it has already received positive feedback. I suggest we wait > until it gets committed, or at most one week.
Committed, thanks. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
