BUG: fast-import, ftruncate, and file mode

BUG: fast-import, ftruncate, and file mode

[Elijah Newren]

Hi,

fast-import makes use of odb_mkstemp(), which creates a secure
temporary file and opens it with mode 0444, and then uses it for its
packfile writing.  Sometimes, fast-import will call its
truncate_pack() function, which makes use of ftruncate().

According to my local manpage, "With ftruncate(), the file must be
open for writing; with truncate(), the file must be writable."

The writable requirement does not appear to be enforced by the kernel
on common filesystems like ext4 or zfs, but this is enforced on some
filesystems.  Apparently a "VxFS Veritas filesystem" got triggered by
this...and some helpful bug reporters tracked this problem down and
found a workaround (for the filter-repo usecase, they recompiled a
special copy of git using mode 0644 for odb_mkstemp, since it was just
an intermediate step anyway and won't be used elsewhere).

I'm not sure if we should (1) just stop calling truncate_pack() in
fast-import (it's merely an optimization), or (2) modify odb_mkstemp()
to allow specifying a mode and also have something like
finalize_object_file() modify the mode before renaming, or (3) if we
should do something else entirely.  Someone more familiar with the
object storage side of things probably knows.  But I'm guessing that
once someone who knows the area states what should be done, that this
might be a small micro-project suitable for the GSoC interns (or
anyone else wanting to get involved)?

Original details from here:
https://github.com/newren/git-filter-repo/issues/342#issuecomment-1047638304

Re: BUG: fast-import, ftruncate, and file mode

[Phillip Wood]

On 23/02/2022 07:47, Elijah Newren wrote:
> Hi,
> 
> fast-import makes use of odb_mkstemp(), which creates a secure
> temporary file and opens it with mode 0444, and then uses it for its
> packfile writing.  Sometimes, fast-import will call its
> truncate_pack() function, which makes use of ftruncate().
> 
> According to my local manpage, "With ftruncate(), the file must be
> open for writing; with truncate(), the file must be writable."
> 
> The writable requirement does not appear to be enforced by the kernel
> on common filesystems like ext4 or zfs, but this is enforced on some
> filesystems.  Apparently a "VxFS Veritas filesystem" got triggered by
> this...and some helpful bug reporters tracked this problem down and
> found a workaround (for the filter-repo usecase, they recompiled a
> special copy of git using mode 0644 for odb_mkstemp, since it was just
> an intermediate step anyway and won't be used elsewhere).

Am I missing something or is this really a file system bug? Surely if we 
have opened a file for writing the file permissions when we call 
ftruncate() should be irrelevant?

Best Wishes

Phillip

> I'm not sure if we should (1) just stop calling truncate_pack() in
> fast-import (it's merely an optimization), or (2) modify odb_mkstemp()
> to allow specifying a mode and also have something like
> finalize_object_file() modify the mode before renaming, or (3) if we
> should do something else entirely.  Someone more familiar with the
> object storage side of things probably knows.  But I'm guessing that
> once someone who knows the area states what should be done, that this
> might be a small micro-project suitable for the GSoC interns (or
> anyone else wanting to get involved)?
> 
> Original details from here:
> https://github.com/newren/git-filter-repo/issues/342#issuecomment-1047638304

Re: BUG: fast-import, ftruncate, and file mode

[Elijah Newren]

On Wed, Feb 23, 2022 at 5:59 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 23/02/2022 07:47, Elijah Newren wrote:
> > Hi,
> >
> > fast-import makes use of odb_mkstemp(), which creates a secure
> > temporary file and opens it with mode 0444, and then uses it for its
> > packfile writing.  Sometimes, fast-import will call its
> > truncate_pack() function, which makes use of ftruncate().
> >
> > According to my local manpage, "With ftruncate(), the file must be
> > open for writing; with truncate(), the file must be writable."
> >
> > The writable requirement does not appear to be enforced by the kernel
> > on common filesystems like ext4 or zfs, but this is enforced on some
> > filesystems.  Apparently a "VxFS Veritas filesystem" got triggered by
> > this...and some helpful bug reporters tracked this problem down and
> > found a workaround (for the filter-repo usecase, they recompiled a
> > special copy of git using mode 0644 for odb_mkstemp, since it was just
> > an intermediate step anyway and won't be used elsewhere).
>
> Am I missing something or is this really a file system bug? Surely if we
> have opened a file for writing the file permissions when we call
> ftruncate() should be irrelevant?
>
> Best Wishes
>
> Phillip

Oh, indeed, looks like I can't read late at night.  Sorry for the noise.

Re: BUG: fast-import, ftruncate, and file mode

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> Am I missing something or is this really a file system bug? Surely if we
>> have opened a file for writing the file permissions when we call
>> ftruncate() should be irrelevant?
>> ...
> Oh, indeed, looks like I can't read late at night.  Sorry for the noise.

Was wondering the same thing.

We open for writing, write one object at a time and when we realize
we do not have to, we rewind that stream we are writing into using
ftruncate().  It would be a serious problem if we cannot do that.