Bug 195436

Summary: patch utility, line number overflows checks
Product: Base System Reporter: David CARLIER <david.carlier>
Component: binAssignee: Pedro F. Giffuni <pfg>
Status: Closed FIXED    
Severity: Affects Only Me CC: op, pfg
Priority: ---    
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Proposed patch
none
Proposed patch
none
Proposed patch
none
Patch based on OpenBSD
none
Patch based on OpenBSD none

Description David CARLIER 2014-11-27 01:05:41 UTC
Created attachment 149926 [details]
Proposed patch

Inspired by OpenBSD's version. it does some overflow checks in case line numbers are beyond of LONG_MAX limits and such.
Comment 1 Pedro F. Giffuni freebsd_committer 2014-11-27 11:22:09 UTC
Hello and thank you;

the patch looks good, however I am traveling at this time and I can't play with it.
Comment 2 David CARLIER 2014-11-27 15:23:19 UTC
Created attachment 149941 [details]
Proposed patch
Comment 3 Pedro F. Giffuni freebsd_committer 2014-11-29 07:54:16 UTC
If the changes come from OpenBSD, it would be good to update the CVS revision information as the tags are used for reference in future updates.
Comment 4 David CARLIER 2014-11-29 09:11:24 UTC
Created attachment 149982 [details]
Proposed patch
Comment 5 Pedro F. Giffuni freebsd_committer 2014-12-05 14:39:27 UTC
Grab this.
Comment 6 Pedro F. Giffuni freebsd_committer 2014-12-07 19:42:03 UTC
Created attachment 150316 [details]
Patch based on OpenBSD

I am inclined to follow the complete OpenBSD change.

This revision precedes your original change:

Introduce strtolinenum to properly check line numbers while parsing:
no signs, no spaces, just digits, 0 <= x <= LONG_MAX
Comment 7 Pedro F. Giffuni freebsd_committer 2014-12-07 21:00:03 UTC
Created attachment 150320 [details]
Patch based on OpenBSD

Update the patch, now that I have merged some previous changes from OpenBSD.
Comment 8 commit-hook freebsd_committer 2014-12-08 15:10:59 UTC
A commit references this bug:

Author: pfg
Date: Mon Dec  8 15:10:49 UTC 2014
New revision: 275612
URL: https://svnweb.freebsd.org/changeset/base/275612

Log:
  patch(1): avoid line number overflows

  Introduce strtolinenum to properly check line numbers while parsing:
  no signs, no spaces, just digits, 0 <= x <= LONG_MAX

  Properly validate line ranges supplied in diff file to prevent overflows.
  Also fixes an out of boundary memory access because the resulting values
  are used as array indices.

  PR:	195436
  Obtained from:	OpenBSD (CVS pch.c rev 1.45, 1,46, common.h rev 1.28)
  MFC after:	1 week

Changes:
  head/usr.bin/patch/common.h
  head/usr.bin/patch/pch.c
Comment 9 Pedro F. Giffuni freebsd_committer 2014-12-08 15:16:00 UTC
Thank you for pointing out the issue.

I went with the OpenBSD patch as it makes it easier to keep in sync (we have been diverging in other ways though).
I didn't update the OpenBSD revision number on the affected files because we are skipping at least one change: not sure if we want to drop the SCCS support just yet (legacy ports exist).
Comment 10 commit-hook freebsd_committer 2014-12-16 21:14:33 UTC
A commit references this bug:

Author: pfg
Date: Tue Dec 16 21:13:56 UTC 2014
New revision: 275841
URL: https://svnweb.freebsd.org/changeset/base/275841

Log:
  MFC	r275553, r275612;

  patch(1): Bring fixes from OpenBSD

  Check fstat return value.  Use off_t for file size and offsets.
  Avoid iterating over end of string.

  Introduce strtolinenum to properly check line numbers while parsing:
  no signs, no spaces, just digits, 0 <= x <= LONG_MAX

  Properly validate line ranges supplied in diff file to prevent overflows.
  Also fixes an out of boundary memory access because the resulting values
  are used as array indices.

  PR:		195436
  Obtained from:	OpenBSD

Changes:
_U  stable/10/
  stable/10/usr.bin/patch/common.h
  stable/10/usr.bin/patch/pch.c
Comment 11 Pedro F. Giffuni freebsd_committer 2014-12-17 00:59:58 UTC
OpenBSD has new changes that will have to be analyzed but this issue can be closed now.