On 27/09/2007, Jean-Pierre André wrote:
> Modified Files:
> Tag: PERMISSION_HANDLING_BRANCH
> security.c
> Log Message:
>
>
Try to add a log message upon commit please. Some people look at it.
> [...2783 lines suppressed...]
I hate when cvs does that. The below review is copied pasted from cvsweb.
In indexsearch():
union {
struct {
u32 dataoffsl;
u32 dataoffsh;
} parts;
u64 all;
} realign;
Not BE safe.
ni = ntfs_pathname_to_inode(scx->vol, NULL, "$Secure");
You already know the MFT number (FILE_Secure). Also, it should be
opened once and cached in scx->vol like other metadata files.
Preferably, $SII and $SDH will be open too.
In is_world_sid():
{
int any;
/* check whether S-1-1-0 */
any = (usid->sub_authority_count == 1)
&& (usid->identifier_authority.high_part == cpu_to_be32(0))
&& (usid->identifier_authority.low_part == cpu_to_be32(1))
&& (usid->sub_authority[0] == cpu_to_le32(0));
return (any);
}
cpu_to_be32(0) is 0. As well as cpu_to_le32(0) (look closely). This is
a matter of taste. I suggest to keep it simple.
Why do you assign to "any"?
In is_user_sid() you check for 5 sub authorities. It may be longer in
case of domain trust.
In attr_size() you declare to assume a DACL is present. I've seen
cases in which it is not true.
In build_permissions() you use:
adminowns = !memcmp(usid,adminsid(),usidsz) || !memcmp(gsid,adminsid(),gsidsz);
You assume that usidsz >= 16. It is unlikely to crash if not, but it
is still an assumption you should not make. (think fuzzing)
In ntfs_get_perm() and ntfs_get_owner_mode():
securattr[le32_to_cpu(phead->owner)]
You are using a tainted index without boundary checking.
Misc:
Why do you wrap constants in worldsid(), adminsid(), systemsid()
instead of declaring a (static) global const?
You assume that a user only has one group. This is true for SFU too.
This is not generally true in either the Windows or the POSIX world.
The patch is too large to review it all. I could see that some of the
notes above returned more than once.
In general, the quality of the code is good. I'd like to see it
integrated after it cooks for a while in a branch. Maybe a in beta
release line?
--
Yuval Fledel
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
ntfs-3g-devel mailing list
ntfs-3g-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
