Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc fixes and cleanup for project quota #7265

Merged
merged 1 commit into from
Mar 5, 2018
Merged

Conversation

Nasf-Fan
Copy link
Contributor

@Nasf-Fan Nasf-Fan commented Mar 4, 2018

  1. The Coverity Scan reports some issues for the project
    quota patch, including:

1.1) zfs_prop_get_userquota() directly uses the const quota
type value as the condition check by wrong.

1.2) dmu_objset_userquota_get_ids() may cause dnode::dn_newgid
to be overwritten by dnode::dn->dn_oldprojid.

  1. This patch fixes related issues. It also enhances the logic
    for zfs_project_item_alloc() to avoid buffer overflow.

  2. Skip project quota ability check if does not change project
    quota related things (id or flag). Otherwise, it will cause
    chattr (for other non project quota flags) operation failed
    if project quota disabled.

Signed-off-by: Fan Yong fan.yong@intel.com

Description

Fix the issues that were introduced by the project quota patch

Motivation and Context

The Coverity Scan reported some issues for the project quota patch, including:

** CID 174157: Incorrect expression (UNUSED_VALUE)
/module/zfs/dmu_objset.c: 2064 in dmu_objset_userquota_get_ids()


*** CID 174157: Incorrect expression (UNUSED_VALUE)
/module/zfs/dmu_objset.c: 2064 in dmu_objset_userquota_get_ids()
2058 * If we don't know what the old values are then just assign
2059 * them to 0, since that is a new file being created.
2060 */
2061 if (!before && data == NULL && error == EEXIST) {
2062 if (flags & DN_ID_OLD_EXIST) {
2063 dn->dn_newuid = dn->dn_olduid;

CID 174157:  Incorrect expression  (UNUSED_VALUE)
Assigning value from "dn->dn_oldgid" to "dn->dn_newgid" here, but that stored value is overwritten before it can be used.

2064 dn->dn_newgid = dn->dn_oldgid;
2065 dn->dn_newgid = dn->dn_oldprojid;
2066 } else {
2067 dn->dn_newuid = 0;
2068 dn->dn_newgid = 0;
2069 dn->dn_newprojid = ZFS_DEFAULT_PROJID;

** CID 174156: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
/lib/libzfs/libzfs_dataset.c: 3180 in zfs_prop_get_userquota()


*** CID 174156: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
/lib/libzfs/libzfs_dataset.c: 3180 in zfs_prop_get_userquota()
3174 return (err);
3175 3176 if (literal) {
3177 (void) snprintf(propbuf, proplen, "%llu",
3178 (u_longlong_t)propvalue);
3179 } else if (propvalue == 0 &&

CID 174156:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
The expression "type == ZFS_PROP_USERQUOTA || type == ZFS_PROP_GROUPQUOTA || type == ZFS_PROP_USEROBJQUOTA || type == ZFS_PROP_GROUPOBJQUOTA || type == ZFS_PROP_PROJECTQUOTA || ZFS_PROP_PROJECTOBJQUOTA" is suspicious because it performs a Boolean operation on a constant other than 0 or 1.

3180 (type == ZFS_PROP_USERQUOTA || type == ZFS_PROP_GROUPQUOTA ||
3181 type == ZFS_PROP_USEROBJQUOTA || type ==
ZFS_PROP_GROUPOBJQUOTA ||
3182 type == ZFS_PROP_PROJECTQUOTA || ZFS_PROP_PROJECTOBJQUOTA)) {
3183 (void) strlcpy(propbuf, "none", proplen);
3184 } else if (type == ZFS_PROP_USERQUOTA || type ==
ZFS_PROP_GROUPQUOTA ||
3185 type == ZFS_PROP_USERUSED || type == ZFS_PROP_GROUPUSED ||

** CID 174154: Security best practices violations (STRING_OVERFLOW)
/cmd/zfs/zfs_project.c: 55 in zfs_project_item_alloc()


*** CID 174154: Security best practices violations (STRING_OVERFLOW)
/cmd/zfs/zfs_project.c: 55 in zfs_project_item_alloc()
49 static void
50 zfs_project_item_alloc(list_t *head, const char *name)
51 {
52 zfs_project_item_t *zpi;
53 54 zpi = safe_malloc(sizeof (zfs_project_item_t));

CID 174154:  Security best practices violations  (STRING_OVERFLOW)
You might overrun the 4096-character fixed-size string "zpi->zpi_name" by copying "name" without checking the length.

55 strcpy(zpi->zpi_name, name);
56 list_insert_tail(head, zpi);
57 }
58 59 static int
60 zfs_project_sanity_check(const char *name, zfs_project_control_t
*zpc,

There is another chatter trouble found in #7251

How Has This Been Tested?

Using the existing test sets

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

1) The Coverity Scan reports some issues for the project
   quota patch, including:

1.1) zfs_prop_get_userquota() directly uses the const quota
   type value as the condition check by wrong.

1.2) dmu_objset_userquota_get_ids() may cause dnode::dn_newgid
   to be overwritten by dnode::dn->dn_oldprojid.

2) This patch fixes related issues. It also enhances the logic
   for zfs_project_item_alloc() to avoid buffer overflow.

3) Skip project quota ability check if does not change project
   quota related things (id or flag). Otherwise, it will cause
   chattr (for other non project quota flags) operation failed
   if project quota disabled.

Signed-off-by: Fan Yong <fan.yong@intel.com>
@codecov
Copy link

codecov bot commented Mar 4, 2018

Codecov Report

Merging #7265 into master will decrease coverage by 0.19%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #7265     +/-   ##
=========================================
- Coverage    76.6%    76.4%   -0.2%     
=========================================
  Files         327      327             
  Lines      103866   103872      +6     
=========================================
- Hits        79563    79361    -202     
- Misses      24303    24511    +208
Flag Coverage Δ
#kernel 76.4% <100%> (-0.07%) ⬇️
#user 65.68% <66.66%> (-0.4%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5666a99...9db5d14. Read the comment docs.

@behlendorf behlendorf merged commit 2705ebf into openzfs:master Mar 5, 2018
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Sep 22, 2019
1) The Coverity Scan reports some issues for the project
   quota patch, including:

1.1) zfs_prop_get_userquota() directly uses the const quota
   type value as the condition check by wrong.

1.2) dmu_objset_userquota_get_ids() may cause dnode::dn_newgid
   to be overwritten by dnode::dn->dn_oldprojid.

2) This patch fixes related issues. It also enhances the logic
   for zfs_project_item_alloc() to avoid buffer overflow.

3) Skip project quota ability check if does not change project
   quota related things (id or flag). Otherwise, it will cause
   chattr (for other non project quota flags) operation failed
   if project quota disabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Fan Yong <fan.yong@intel.com>
Closes openzfs#7251 
Closes openzfs#7265
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Sep 22, 2019
1) The Coverity Scan reports some issues for the project
   quota patch, including:

1.1) zfs_prop_get_userquota() directly uses the const quota
   type value as the condition check by wrong.

1.2) dmu_objset_userquota_get_ids() may cause dnode::dn_newgid
   to be overwritten by dnode::dn->dn_oldprojid.

2) This patch fixes related issues. It also enhances the logic
   for zfs_project_item_alloc() to avoid buffer overflow.

3) Skip project quota ability check if does not change project
   quota related things (id or flag). Otherwise, it will cause
   chattr (for other non project quota flags) operation failed
   if project quota disabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Fan Yong <fan.yong@intel.com>
Closes openzfs#7251 
Closes openzfs#7265
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Sep 22, 2019
1) The Coverity Scan reports some issues for the project
   quota patch, including:

1.1) zfs_prop_get_userquota() directly uses the const quota
   type value as the condition check by wrong.

1.2) dmu_objset_userquota_get_ids() may cause dnode::dn_newgid
   to be overwritten by dnode::dn->dn_oldprojid.

2) This patch fixes related issues. It also enhances the logic
   for zfs_project_item_alloc() to avoid buffer overflow.

3) Skip project quota ability check if does not change project
   quota related things (id or flag). Otherwise, it will cause
   chattr (for other non project quota flags) operation failed
   if project quota disabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Fan Yong <fan.yong@intel.com>
Closes openzfs#7251 
Closes openzfs#7265
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Sep 22, 2019
1) The Coverity Scan reports some issues for the project
   quota patch, including:

1.1) zfs_prop_get_userquota() directly uses the const quota
   type value as the condition check by wrong.

1.2) dmu_objset_userquota_get_ids() may cause dnode::dn_newgid
   to be overwritten by dnode::dn->dn_oldprojid.

2) This patch fixes related issues. It also enhances the logic
   for zfs_project_item_alloc() to avoid buffer overflow.

3) Skip project quota ability check if does not change project
   quota related things (id or flag). Otherwise, it will cause
   chattr (for other non project quota flags) operation failed
   if project quota disabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Fan Yong <fan.yong@intel.com>
Closes openzfs#7251 
Closes openzfs#7265
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Sep 22, 2019
1) The Coverity Scan reports some issues for the project
   quota patch, including:

1.1) zfs_prop_get_userquota() directly uses the const quota
   type value as the condition check by wrong.

1.2) dmu_objset_userquota_get_ids() may cause dnode::dn_newgid
   to be overwritten by dnode::dn->dn_oldprojid.

2) This patch fixes related issues. It also enhances the logic
   for zfs_project_item_alloc() to avoid buffer overflow.

3) Skip project quota ability check if does not change project
   quota related things (id or flag). Otherwise, it will cause
   chattr (for other non project quota flags) operation failed
   if project quota disabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Fan Yong <fan.yong@intel.com>
Closes openzfs#7251 
Closes openzfs#7265
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Sep 22, 2019
1) The Coverity Scan reports some issues for the project
   quota patch, including:

1.1) zfs_prop_get_userquota() directly uses the const quota
   type value as the condition check by wrong.

1.2) dmu_objset_userquota_get_ids() may cause dnode::dn_newgid
   to be overwritten by dnode::dn->dn_oldprojid.

2) This patch fixes related issues. It also enhances the logic
   for zfs_project_item_alloc() to avoid buffer overflow.

3) Skip project quota ability check if does not change project
   quota related things (id or flag). Otherwise, it will cause
   chattr (for other non project quota flags) operation failed
   if project quota disabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Fan Yong <fan.yong@intel.com>
Closes openzfs#7251 
Closes openzfs#7265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants