On Thu, Sep 22, 2005 at 05:41:40PM +0200, Jan Engelhardt wrote:
>
> >> --- unionfs-20050920-1539/inode.cO 2005-09-21 04:39:03.000000000 +0900
> >> +++ unionfs-20050920-1539/inode.c 2005-09-21 20:10:16.819341832 +0900
> >> @@ -41,7 +41,7 @@
> >> hidden_dentry = dtohd(dentry);
> >>
> >> /* check if whiteout exists in this branch, i.e. lookup .wh.foo first */
> >> - name = KMALLOC(dentry->d_name.len + sizeof(".wh"), GFP_UNIONFS);
> >> + name = KMALLOC(dentry->d_name.len + sizeof(".wh."), GFP_UNIONFS);
> >
> >I can't believe this. I have been reading that special part of inode.c
> >several times during my 3 week debugging session, and did not notice the
> >"off by one", maybe because the last byte of "name" is overwritten with
> >a zero later. Also, I was searching more for uninitialized pointers or
> >race conditions.
>
> But! sizeof(".wh.") is actually 5
Correct, because string constants contain the zero.
One could now argue that the fact that the code with ".wh." does not
crash opposed to the one with ".wh" only, but we have a more logic
explanation caused by the POSIC definition of strncat().
> so... is it that we allocate one byte
> too much now? (That would be the "string + size parameter" variant, as
> opposed to "zero-terminated string", such as in maybe readlink().)
Also makes sense, but: Old code crashes, new code works. It may be
explainable because of strcat and strcmp which honour trailing zeros
even if the length-dependent versions are used:
"The strncat() function is similar [to strcat], except that it will
use at most n characters from src. Since the result is always terminated
with `\0', at most n+1 characters are written."
That means that we can indeed have a "off by one" overwrite of data
caused by the fact that we allocated space for ".wh", but added a ".wh."
and an additional zero at the end of the entire string, regardless of
the fact that the original filename may not even have been 0-terminated
because of new_dentry->d_name.len containing the length of the real
filename.
Regards
-Klaus Knopper
_______________________________________________
unionfs mailing list
[email protected]
http://www.fsl.cs.sunysb.edu/mailman/listinfo/unionfs
