Bug 243974 - diff: generates core dump when --tabsize is used
Summary: diff: generates core dump when --tabsize is used
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.1-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: Kyle Evans
URL:
Keywords: crash
: 241714 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-02-08 09:18 UTC by fehmi noyan isi
Modified: 2020-03-22 01:03 UTC (History)
4 users (show)

See Also:
kevans: mfc-stable12?
kevans: mfc-stable11?
kevans: mfc-stable10-


Attachments
patch for diff.c (629 bytes, patch)
2020-02-09 08:58 UTC, fehmi noyan isi
no flags Details | Diff
git(1) diff against base (3.02 KB, patch)
2020-02-13 15:47 UTC, Kyle Evans
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description fehmi noyan isi 2020-02-08 09:18:15 UTC
diff(1) causes SIGSEGV when --tabsize is used. Problem is present on CURRENT as well

fnoyanisi@beastie:~ % diff A B 
1,5c1,3
< same1
< differ1
< 
< 
< differ3
---
> same1  
> differ2
> differ4
fnoyanisi@beastie:~ % diff A B --tabsize 4
Segmentation fault (core dumped)
fnoyanisi@beastie:~ % uname -a
FreeBSD beastie 12.1-RELEASE-p2 FreeBSD 12.1-RELEASE-p2 GENERIC  amd64

I am working on a patch, checking out the latest head and building the world at the moment.
Comment 1 fehmi noyan isi 2020-02-08 09:44:40 UTC
duplicate of 

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241714

The resolution mentioned in the above bug report is to pass the argument following --tabsize after an equal sign, e,g, --tabsize=NUMBER as opposed to --tabsize NUMBER.

In diff.c, the argument to tabsize is classified as "optional_argument" whereas it should be "required_argument". 

We either should;
- update the man page and indicate the argument following --tabsize is optional
- make the argument to --tabsize required. 

I am more keen to go with the second option (i.e. making the argument mandatory for the --tabsize). It does not make much sense to use only --tabsize
Comment 2 fehmi noyan isi 2020-02-09 08:58:23 UTC
Created attachment 211493 [details]
patch for diff.c

this patch changes the NUMBER argument following --tabsize from optional to required.
Comment 3 fehmi noyan isi 2020-02-13 07:11:54 UTC
POC for the proposed patch

fnoyanisi@patch:~ $ diff A B
1,5c1,3
< same1
< differ1
< 
< 
< differ3
---
> same1  
> differ2
> differ4
fnoyanisi@patch:~ $ diff A B --tabsize 4
Segmentation fault (core dumped)

// After the patch
fnoyanisi@patch:~ $ diff A B --tabsize 4
1,5c1,3
< same1
< differ1
< 
< 
< differ3
---
> same1  
> differ2
> differ4
fnoyanisi@patch:~ $ uname -a
FreeBSD patch 13.0-CURRENT FreeBSD 13.0-CURRENT #0 r357606: Thu Feb  6 04:40:35 UTC 2020     root@releng1.nyi.freebsd.org:/usr/obj/usr/src/amd64.amd64/sys/GENERIC  amd64
fnoyanisi@patch:~ $ 

// No failing ATF tests
root@patch:/usr/tests # kyua test -k /usr/tests/Kyuafile usr.bin/diff
usr.bin/diff/diff_test:Bflag  ->  passed  [0.191s]
usr.bin/diff/diff_test:b230049  ->  passed  [0.054s]
usr.bin/diff/diff_test:brief_format  ->  passed  [0.180s]
usr.bin/diff/diff_test:group_format  ->  passed  [0.064s]
usr.bin/diff/diff_test:header  ->  passed  [0.066s]
usr.bin/diff/diff_test:header_ns  ->  passed  [0.063s]
usr.bin/diff/diff_test:ifdef  ->  passed  [0.060s]
usr.bin/diff/diff_test:side_by_side  ->  passed  [0.130s]
usr.bin/diff/diff_test:simple  ->  passed  [0.230s]
usr.bin/diff/diff_test:unified  ->  passed  [0.097s]
usr.bin/diff/netbsd_diff_test:mallocv  ->  passed  [0.069s]
usr.bin/diff/netbsd_diff_test:nomallocv  ->  passed  [0.070s]
usr.bin/diff/netbsd_diff_test:same  ->  passed  [0.071s]

Results file id is usr_tests.20200213-193818-429125
Results saved to /root/.kyua/store/results.usr_tests.20200213-193818-429125.db

13/13 passed (0 failed)
root@patch:/usr/tests # 

There is another diff(1) patch I am working on [1] but I am reluctant to submit more than one patch for the same source file at once.

[1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=243975
Comment 4 Kyle Evans freebsd_committer 2020-02-13 15:47:13 UTC
Created attachment 211617 [details]
git(1) diff against base

I intend to commit the attached diff a little later today, Submitted by: fehmi noyan isi <fnoyanisi yahoo com> (diff.c portion) -- it also consistently shows the argument to tabsize in the man page and adds a test case for diff --tabsize.
Comment 5 commit-hook freebsd_committer 2020-02-13 20:24:43 UTC
A commit references this bug:

Author: kevans
Date: Thu Feb 13 20:23:56 UTC 2020
New revision: 357875
URL: https://svnweb.freebsd.org/changeset/base/357875

Log:
  diff: fix segfault with --tabsize and no/malformed argument

  --tabsize was previously listed as optional_argument, but didn't account for
  the optionality of it in the argument handling. This is irrelevant -- the
  manpage doesn't indicate that the argument is optional, and indeed there's
  no clear interpretation of omitting the argument because there's no other
  side effect of --tabsize.

  The "malformed" argument part of the header on this message is simply
  referring to usage like this:

  % diff --tabsize 4 A B

  With an optional_argument, the argument must be attached to the parameter
  directly (e.g. --tabsize=4), so the argument is effectively NULL with the
  above invocation as if no argument had been passed.

  PR:		243974
  Submitted by:	fehmi noyan isi <fnoyanisi yahoo com> (diff.c portion)
  MFC after:	3 days

Changes:
  head/usr.bin/diff/diff.1
  head/usr.bin/diff/diff.c
  head/usr.bin/diff/tests/diff_test.sh
Comment 6 Kubilay Kocak freebsd_committer freebsd_triage 2020-03-22 01:01:32 UTC
^Triage: Assign to committer that resolved. Pending MFC('s).
Comment 7 Kubilay Kocak freebsd_committer freebsd_triage 2020-03-22 01:03:06 UTC
*** Bug 241714 has been marked as a duplicate of this bug. ***