Bug 225162 - Source file zfs_acl.c, function zfs_aclset_common contains a use after end of the lifetime of a local variable
Summary: Source file zfs_acl.c, function zfs_aclset_common contains a use after end of...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Andriy Gapon
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-01-14 17:48 UTC by WHR
Modified: 2018-03-01 08:36 UTC (History)
3 users (show)

See Also:


Attachments
Patch (469 bytes, patch)
2018-01-14 17:48 UTC, WHR
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WHR 2018-01-14 17:48:20 UTC
Created attachment 189714 [details]
Patch

Source file https://svnweb.freebsd.org/base/head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c (latest version r323491 at this time), line 1220, in function zfs_aclset_common have a local variable definition "zfs_acl_phys_t acl_phys;". At line 1297, the pointer to this variable (&acl_phys) is stored into the array "bulk"; then the current code block and the lifetime of "acl_phys" is ended after this, but "bulk" is still got used at line 1314.

This code resulted in undefined behavior, meaning this bug may not be generally noticeable. In my test, the clang 3.4.1 on FreeBSD 10.3 amd64 won't trigger wrong behavior; however gcc 4.7 4.8 4.9 at any optimization level (except "-O0") will resulting a buggy behavior which showing to the user as:

[WHR@kmod-test /testpool]$ mkdir 35
[WHR@kmod-test /testpool]$ cd 35
-bash: cd: 35: Permission denied

Due the ACL is failed to store.

The attached patch will fix this bug by moving the definition of "acl_phys" to the top block of the function, thus its lifetime will cover the whole function.
Comment 1 WHR 2018-01-14 17:49:05 UTC
Source file https://svnweb.freebsd.org/base/head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c (latest version r323491 at this time), line 1220, in function zfs_aclset_common have a local variable definition "zfs_acl_phys_t acl_phys;". At line 1297, the pointer to this variable (&acl_phys) is stored into the array "bulk"; then the current code block and the lifetime of "acl_phys" is ended after this, but "bulk" is still got used at line 1314.

This code resulted in undefined behavior, meaning this bug may not be generally noticeable. In my test, the clang 3.4.1 on FreeBSD 10.3 amd64 won't trigger wrong behavior; however gcc 4.7 4.8 4.9 at any optimization level (except "-O0") will resulting a buggy behavior which showing to the user as:

[WHR@kmod-test /testpool]$ mkdir 35
[WHR@kmod-test /testpool]$ cd 35
-bash: cd: 35: Permission denied

Due the ACL is failed to store.

The attached patch will fix this bug by moving the definition of "acl_phys" to the top block of the function, thus its lifetime will cover the whole function.
Comment 2 Yuri Pankov 2018-01-14 21:06:59 UTC
Reported upstream: https://www.illumos.org/issues/8966
Comment 3 commit-hook freebsd_committer freebsd_triage 2018-02-21 14:18:03 UTC
A commit references this bug:

Author: avg
Date: Wed Feb 21 14:17:07 UTC 2018
New revision: 329711
URL: https://svnweb.freebsd.org/changeset/base/329711

Log:
  MFV r329710: 8966 Source file zfs_acl.c, function zfs_aclset_common contains a use after end of the lifetime of a local variable

  illumos/illumos-gate@82693e09cc02331fa1b3b73b54b1060e73507a8d
  https://github.com/illumos/illumos-gate/commit/82693e09cc02331fa1b3b73b54b1060e73507a8d
  https://www.illumos.org/issues/8966

  Reviewed by: Matt Ahrens <mahrens@delphix.com>
  Reviewed by: Andriy Gapon <avg@FreeBSD.org>
  Approved by: Richard Lowe <richlowe@richlowe.net>
  Author: WHR <msl0000023508@gmail.com>
  PR:		225162
  Submitted by:	WHR <msl0000023508@gmail.com>
  Reported by:	WHR <msl0000023508@gmail.com>
  MFC after:	1 week

Changes:
_U  head/sys/cddl/contrib/opensolaris/
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c
Comment 4 commit-hook freebsd_committer freebsd_triage 2018-03-01 08:31:03 UTC
A commit references this bug:

Author: avg
Date: Thu Mar  1 08:30:09 UTC 2018
New revision: 330234
URL: https://svnweb.freebsd.org/changeset/base/330234

Log:
  MFC r329711: MFV r329710: 8966 use after end of the lifetime of a local variable

  PR:		225162

Changes:
_U  stable/10/
  stable/10/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c
Comment 5 commit-hook freebsd_committer freebsd_triage 2018-03-01 08:34:08 UTC
A commit references this bug:

Author: avg
Date: Thu Mar  1 08:33:20 UTC 2018
New revision: 330235
URL: https://svnweb.freebsd.org/changeset/base/330235

Log:
  MFC r329711: MFV r329710: 8966 use after end of the lifetime of a local variable

  PR:		225162

Changes:
_U  stable/11/
  stable/11/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c