Bug 12567 - Redundant checks in samba-4.5.5/source3/smbd/posix_acls.c
Redundant checks in samba-4.5.5/source3/smbd/posix_acls.c
Status: ASSIGNED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other
4.5.5
All All
: P5 trivial
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-07 07:36 UTC by shqking
Modified: 2017-02-10 07:28 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description shqking 2017-02-07 07:36:35 UTC
Redundant checks are found in samba-4.5.5/source3/smbd/posix_acls.c.
The code snippet is as follows.

  895: 	if (sys_acl_clear_perms(*p_permset) ==  -1)
  896  		return -1;

 4250: 	if (sys_acl_clear_perms(*p_permset) ==  -1) {
 4251  		return False;
 4252  	}

The return value of function "sys_acl_clear_perms" is checked with "-1" at line 895 and line 4250.

Note that the definition for function "sys_acl_clear_perms" is at samba-4.5.5/source3/lib/sysacls.c:117, shown as below.

  int sys_acl_clear_perms(SMB_ACL_PERMSET_T permset_d)
  {
	*permset_d = 0;

	return 0;
  }

We can find that function "sys_acl_clear_perms" can only return "0" and hence the checks at line 895 and line 4250 are redundant and unnecessary.

Strictly speaking, this issue is not a bug, but a software defect.

Thanks.
Comment 1 Jeremy Allison 2017-02-09 21:03:46 UTC
sys_acl_clear perms is meant to wrap the libacl function acl_clear_perms().

The man page for that states:

     int
     acl_clear_perms(acl_permset_t permset_d);

RETURN VALUE
     The acl_clear_perms() function returns the value 0 if successful; otherwise the value -1 is returned
     and the global variable errno is set to indicate the error.

ERRORS
     If any of the following conditions occur, the acl_clear_perms() function returns -1 and sets errno to
     the corresponding value:

     [EINVAL]           The argument permset_d is not a valid descriptor for a permission set within an ACL
                        entry.

So if our implementation called the underlying system acl_clear_perms() function (as arguably it should) then there is a possible error here.

Let me think about the possible correct fix here..
Comment 2 shqking 2017-02-10 07:28:28 UTC
(In reply to Jeremy Allison from comment #1)
Hi Jeremy,
Thanks for your reply.

Two more issues, which are quite similar with current post, are found.
I put them under this post, and you may want to fix them together.  
Thanks a lot. :)



==========================
Issue #1:
The return value of function "sys_acl_get_tag_type" is checked with "-1" in the following sites. However, it can only return "0" according to its definition (at /samba-4.5.5/source3/lib/sysacls.c:89).

/samba-4.5.5/source3/smbd/posix_acls.c:
 2625  	entry_id = SMB_ACL_NEXT_ENTRY;
 2626  
 2627: 	if (sys_acl_get_tag_type(entry, &tagtype) == -1)
 2628  		continue;
 2629  
 ....
 4052  	entry_id = SMB_ACL_NEXT_ENTRY;
 4053  
 4054: 	if (sys_acl_get_tag_type(entry, &tagtype) ==-1)
 4055  		break;
 4056  
 ....
 4090  	entry_id = SMB_ACL_NEXT_ENTRY;
 4091  
 4092: 	if (sys_acl_get_tag_type(entry, &tagtype) == -1)
 4093  		return -1;
 4094  
 ....
 4525  	entry_id = SMB_ACL_NEXT_ENTRY;
 4526  
 4527: 	if (sys_acl_get_tag_type(entry, &tagtype) == -1) {
 4528  		DEBUG(5,("remove_posix_acl: failed to get tagtype from ACL on file %s (%s).\n",
 4529  		      fname, strerror(errno) ));

/samba-4.5.5/source3/smbd/trans2.c:
 4377  	}
 4378  
 4379: 	if (sys_acl_get_tag_type(entry, &tagtype) == -1) {
 4380: 		DEBUG(0,("marshall_posix_acl: SMB_VFS_SYS_ACL_GET_TAG_TYPE failed.\n"));
 4381  		return False;
 4382  		}

==========================
Issue #2:
The return value of function "sys_acl_get_permset" is checked with "-1" in the following sites. However, it can only return "0" according to its definition (at /samba-4.5.5/source3/lib/sysacls.c:96).

/samba-4.5.5/source3/smbd/posix_acls.c:
 2628  		continue;
 2629  
 2630: 	if (sys_acl_get_permset(entry, &permset) == -1)
 2631  		continue;
 2632  
 ....
 2911  	 */
 2912  
 2913: 	if (sys_acl_get_permset(the_entry, &the_permset) == -1) {
 2914  		DEBUG(0,("set_canon_ace_list: Failed to get permset on entry %d. (%s)\n",
 2915  			i, strerror(errno) ));
 ....
 2949  	}
 2950  
 2951: 	if (sys_acl_get_permset(mask_entry, &mask_permset) == -1) {
 2952  		DEBUG(0,("set_canon_ace_list: Failed to get mask permset. (%s)\n", strerror(errno) ));
 2953  		goto fail;
 ....
 4056  
 4057  	if (tagtype == SMB_ACL_GROUP_OBJ) {
 4058: 		if (sys_acl_get_permset(entry, &permset) == -1) {
 4059  			break;
 4060  		} else {
 ....
 4093  		return -1;
 4094  
 4095: 	if (sys_acl_get_permset(entry, &permset) == -1)
 4096  		return -1;
 4097  
 ....
 4342  
 4343  	/* Get the permset pointer from the new ACL entry. */
 4344: 	if (sys_acl_get_permset(the_entry, &the_permset) == -1) {
 4345  		DEBUG(0,("create_posix_acl_from_wire: Failed to get permset on entry %u. (%s)\n",
 4346                i, strerror(errno) ));
 ....
 4531  	}
 4532  
 4533: 	if (sys_acl_get_permset(entry, &permset) == -1) {
 4534  		DEBUG(5,("remove_posix_acl: failed to get permset from ACL on file %s (%s).\n",
 4535  			fname, strerror(errno) ));

/samba-4.5.5/source3/smbd/trans2.c:
 4382  	}
 4383  
 4384: 	if (sys_acl_get_permset(entry, &permset) == -1) {
 4385: 		DEBUG(0,("marshall_posix_acl: SMB_VFS_SYS_ACL_GET_PERMSET failed.\n"));
 4386  		return False;
 4387  	}