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.
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..
(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 }