Hi,
I think I may have found a bug in fs/ufs/inode.c
lines 334 & 335 have this code (in function ufs_getfrag_block) :
if (fragment < 0)
goto abort_negative;
fragment is an argument to ufs_getfrag_block of type sector_t
digging a little in include/linux and include/asm reveals sector_t to be
an an unsigned type in all archs (at least as far as I've been able to
determine).
include/linux/types.h defines sector_t as
typedef unsigned long sector_t;
and in include/asm* sector_t is defined as :
asm-sh/types.h:typedef u64 sector_t;
asm-x86_64/types.h:typedef u64 sector_t;
asm-h8300/types.h:typedef u64 sector_t;
asm-i386/types.h:typedef u64 sector_t;
asm-mips/types.h:typedef u64 sector_t;
asm-s390/types.h:typedef u64 sector_t;
asm-ppc/types.h:typedef u64 sector_t;
all unsigned types.
So, since sector_t is unsigned it can never be less than zero, hence the
branch on line 334 will never be taken and the assiciated label
"abort_negative" will never be reached (and it's not referenced anywhere
else).
I don't know enough about UFS to be able to tell if this is actually a
problem or not, but I suspect that it might be.. If it's not a problem,
then the code that tests for (fragment < 0) and the associated code
in abort_negative might as well be removed.
Is this analysis correct? If it is, can the code simply be removed?
Kind regards,
Jesper Juhl
Hmm, the exact same thing as below seems to be going on in
fs/adfs/inode.c - line 30
fs/befs/linuxvfs.c - line 135
fs/bfs/file.c - line 67 (this one's a little different, but the sector_t < 0 check is involved
fs/reiserfs/inode.c - line 577
should I be worried? Is this worth digging into any further?
I don't mind doing some intensive fs code studying, but before I commit a
lot of time to this it would be nice with a confirmation from someone who
already knows this stuff that it's worth spending time on... I don't yet
know enough to judge that..
/Jesper Juhl
On Sat, 3 Jan 2004, Jesper Juhl wrote:
>
> Hi,
>
> I think I may have found a bug in fs/ufs/inode.c
>
> lines 334 & 335 have this code (in function ufs_getfrag_block) :
>
> if (fragment < 0)
> goto abort_negative;
>
> fragment is an argument to ufs_getfrag_block of type sector_t
> digging a little in include/linux and include/asm reveals sector_t to be
> an an unsigned type in all archs (at least as far as I've been able to
> determine).
>
> include/linux/types.h defines sector_t as
>
> typedef unsigned long sector_t;
>
> and in include/asm* sector_t is defined as :
>
> asm-sh/types.h:typedef u64 sector_t;
> asm-x86_64/types.h:typedef u64 sector_t;
> asm-h8300/types.h:typedef u64 sector_t;
> asm-i386/types.h:typedef u64 sector_t;
> asm-mips/types.h:typedef u64 sector_t;
> asm-s390/types.h:typedef u64 sector_t;
> asm-ppc/types.h:typedef u64 sector_t;
>
> all unsigned types.
>
> So, since sector_t is unsigned it can never be less than zero, hence the
> branch on line 334 will never be taken and the assiciated label
> "abort_negative" will never be reached (and it's not referenced anywhere
> else).
>
> I don't know enough about UFS to be able to tell if this is actually a
> problem or not, but I suspect that it might be.. If it's not a problem,
> then the code that tests for (fragment < 0) and the associated code
> in abort_negative might as well be removed.
>
> Is this analysis correct? If it is, can the code simply be removed?
>
>
> Kind regards,
>
> Jesper Juhl
On Sat, Jan 03, 2004 at 11:45:56PM +0100, Jesper Juhl wrote:
>
> Is this analysis correct? If it is, can the code simply be removed?
It does seem odd, but indeed, a confirmation from someone with authority
would be nice before digging in to a cleanup-process like this. Try e-mailing
the maintainer of the code directly, since it doesn't seem like anyone
feels like wasting any time on this :)
Nice job on the discovery though, if this is true, these things really should
be removed.
--
Tim Cambrant <[email protected]>
GPG KeyID 0x59518702
Fingerprint: 14FE 03AE C2D1 072A 87D0 BC4D FA9E 02D8 5951 8702
On Sun, 4 Jan 2004, Tim Cambrant wrote:
> On Sat, Jan 03, 2004 at 11:45:56PM +0100, Jesper Juhl wrote:
> >
> > Is this analysis correct? If it is, can the code simply be removed?
>
> It does seem odd, but indeed, a confirmation from someone with authority
> would be nice before digging in to a cleanup-process like this. Try
> e-mailing
> the maintainer of the code directly, since it doesn't seem like anyone
> feels like wasting any time on this :)
>
I'm glad I'm not the only one who thinks this is odd. I'll try
emailing the maintainers of the filesystems directly and see what response
I get.
> Nice job on the discovery though,
Thank you.
> if this is true, these things really
> should
> be removed.
>
I just want to find out what is actually going on and why that code was
put there in the first place before I start changing/removing anything.
Currently I'm digging on my own but haven't come up with much yet - except
that the same code seems to be used in some other filesystems as well (as
I note in a reply to my initial mail)...
I'll keep digging.
/Jesper Juhl