This generally good, but you'll need to fix formatting a bit
for both the mail body and the patch itself.
On Mon, Oct 17, 2011 at 01:30:12PM -0200, Carlos Maiolino wrote:
> Fixes a possible memory corruption when the link
> is larger than MAXPATHLEN and XFS_DEBUG is not
> enabled.
> This also uses S_IFLNK to check link not only
> in DEBUG mode.
Please try to fill up ~ 75 characters for each line in the mail body,
e.g.
Fix a possible memory corruption when a symlink target is larger than
MAXPATHLEN and XFS_DEBUG is not enabled. Also use S_IFLNK to check
against disk corruption in di_mode for non-debug mode.
(I've also update the content a little bit).
> - ASSERT(S_ISLNK(ip->i_d.di_mode));
> - ASSERT(ip->i_d.di_size <= MAXPATHLEN);
> + if (!(S_ISLNK(ip->i_d.di_mode)) || !(ip->i_d.di_size <= MAXPATHLEN )){
> +
> + xfs_emerg(mp, "inode (%lld), link too long or not a link",
> + (unsigned long long)ip->i_ino);
> + ASSERT(0);
> + return XFS_ERROR(EFSCORRUPTED);
> + }
No need for the inner braces in both branches, but per kernel coding
style there should be one before the opening brace. Also no spaces
before the closing round braces, please. I also think it would be
cleanrer to split this into two checks, as it's two possible
corruptions, e.g.
if (!S_ISLNK(ip->i_d.di_mode)) {
xfs_emerg(mp, "inode (%lld) not a link in %s\n",
(unsigned long long)ip->i_ino), __func__);
ASSERT(0);
return XFS_ERROR(EFSCORRUPTED);
}
if (ip->i_d.di_size > MAXPATHLEN) {
xfs_emerg(mp, "inode (%lld) larger than MAXPATHLEN in %s\n",
(unsigned long long)ip->i_ino), __func__);
ASSERT(0);
return XFS_ERROR(EFSCORRUPTED);
}
It might also be useful to print the length in the second case as that
would help debugging potential corruptions. (e.g. single bit flips)
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
