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. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
