[PATCH] make pack-objects a bit more resilient to repo corruption

[PATCH] make pack-objects a bit more resilient to repo corruption

[Nicolas Pitre]

Right now, packing valid objects could fail when creating a thin pack 
simply because a pack edge object used as a preferred base is corrupted.
Since preferred base objects are not strictly needed to produce a valid
pack, let's not consider the inability to read them as a fatal error.
Delta compression may well be attempted against other objects in the
search window.

Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
---

I wrote this patch while helping Uwe with his repo corruption.  While 
his problem turned out to be different from the one this patch is 
addressing (see previous patch I posted) I think that since I did the 
patch already then this wouldn't hurt to have this one merged too.

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f8eba53..674247e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1298,9 +1298,19 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 		read_lock();
 		src->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);
 		read_unlock();
-		if (!src->data)
+		if (!src->data) {
+			if (src_entry->preferred_base) {
+				/* 
+				 * Those objects are not included in the
+				 * resulting pack.  Be resilient and ignore
+				 * them if they can't be read, in case the
+				 * pack could be created nevertheless.
+				 */
+				return 0;
+			}
 			die("object %s cannot be read",
 			    sha1_to_hex(src_entry->idx.sha1));
+		}
 		if (sz != src_size)
 			die("object %s inconsistent object length (%lu vs %lu)",
 			    sha1_to_hex(src_entry->idx.sha1), sz, src_size);

Re: [PATCH] make pack-objects a bit more resilient to repo corruption

[Jeff King]

On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:

> -		if (!src->data)
> +		if (!src->data) {
> +			if (src_entry->preferred_base) {
> +				/* 
> +				 * Those objects are not included in the
> +				 * resulting pack.  Be resilient and ignore
> +				 * them if they can't be read, in case the
> +				 * pack could be created nevertheless.
> +				 */
> +				return 0;
> +			}
>  			die("object %s cannot be read",
>  			    sha1_to_hex(src_entry->idx.sha1));
> +		}

By converting this die() into a silent return, are we losing a place
where git might previously have alerted a user to corruption? In this
case, we can continue the operation without the object, but if we have
detected corruption, letting the user know as soon as possible is
probably a good idea.

In other words, should this instead be:

  warning("unable to read preferred base object: %s", ...);
  return 0;

Or will some other part of the code already complained to stderr?

-Peff

Re: [PATCH] make pack-objects a bit more resilient to repo corruption

[Drew Northup]

On Fri, 2010-10-22 at 10:46 -0400, Jeff King wrote:
> On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:
> 
> > -		if (!src->data)
> > +		if (!src->data) {
> > +			if (src_entry->preferred_base) {
> > +				/* 
> > +				 * Those objects are not included in the
> > +				 * resulting pack.  Be resilient and ignore
> > +				 * them if they can't be read, in case the
> > +				 * pack could be created nevertheless.
> > +				 */
> > +				return 0;
> > +			}
> >  			die("object %s cannot be read",
> >  			    sha1_to_hex(src_entry->idx.sha1));
> > +		}
> 
> By converting this die() into a silent return, are we losing a place
> where git might previously have alerted a user to corruption? In this
> case, we can continue the operation without the object, but if we have
> detected corruption, letting the user know as soon as possible is
> probably a good idea.
> 
> In other words, should this instead be:
> 
>   warning("unable to read preferred base object: %s", ...);
>   return 0;
> 
> Or will some other part of the code already complained to stderr?
> 
> -Peff

Agreed. If it broke we should probably tell the user--even if we can't
do much useful about it other than attempt to recover by continuing.

-- 
-Drew Northup N1XIM
   AKA RvnPhnx on OPN
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [PATCH] make pack-objects a bit more resilient to repo corruption

[Nicolas Pitre]

On Fri, 22 Oct 2010, Drew Northup wrote:

> 
> On Fri, 2010-10-22 at 10:46 -0400, Jeff King wrote:
> > On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:
> > 
> > > -		if (!src->data)
> > > +		if (!src->data) {
> > > +			if (src_entry->preferred_base) {
> > > +				/* 
> > > +				 * Those objects are not included in the
> > > +				 * resulting pack.  Be resilient and ignore
> > > +				 * them if they can't be read, in case the
> > > +				 * pack could be created nevertheless.
> > > +				 */
> > > +				return 0;
> > > +			}
> > >  			die("object %s cannot be read",
> > >  			    sha1_to_hex(src_entry->idx.sha1));
> > > +		}
> > 
> > By converting this die() into a silent return, are we losing a place
> > where git might previously have alerted a user to corruption? In this
> > case, we can continue the operation without the object, but if we have
> > detected corruption, letting the user know as soon as possible is
> > probably a good idea.
> > 
> > In other words, should this instead be:
> > 
> >   warning("unable to read preferred base object: %s", ...);
> >   return 0;
> > 
> > Or will some other part of the code already complained to stderr?
> > 
> > -Peff
> 
> Agreed. If it broke we should probably tell the user--even if we can't
> do much useful about it other than attempt to recover by continuing.

Please don't misinterpret this case.  As far as this change is 
concerned, nothing is actually "broken".  The operation _will_ still 
succeed.  The repository may be broken, but in this case we can do 
without the broken object.  In those cases where the object is really 
needed the original die() is still in place.


Nicolas

Re: [PATCH] make pack-objects a bit more resilient to repo corruption

[Nicolas Pitre]

On Fri, 22 Oct 2010, Jeff King wrote:

> On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:
> 
> > -		if (!src->data)
> > +		if (!src->data) {
> > +			if (src_entry->preferred_base) {
> > +				/* 
> > +				 * Those objects are not included in the
> > +				 * resulting pack.  Be resilient and ignore
> > +				 * them if they can't be read, in case the
> > +				 * pack could be created nevertheless.
> > +				 */
> > +				return 0;
> > +			}
> >  			die("object %s cannot be read",
> >  			    sha1_to_hex(src_entry->idx.sha1));
> > +		}
> 
> By converting this die() into a silent return, are we losing a place
> where git might previously have alerted a user to corruption? In this
> case, we can continue the operation without the object, but if we have
> detected corruption, letting the user know as soon as possible is
> probably a good idea.
> 
> In other words, should this instead be:
> 
>   warning("unable to read preferred base object: %s", ...);
>   return 0;

Well, this get called repeatedly, being within the inner part of the 
delta search loop.  So you might get that warning as many times as the 
delta window which is not that nice.  If anything a static flag to 
display the warning only once would be needed.  But you're pretty likely 
to have met that warning/error already from other operations, which is 
why I didn't bother.

> Or will some other part of the code already complained to stderr?

Some other part is likely to already have complained, through 
check_object() -> sha1_object_info().  But not necessarily in all cases.


Nicolas

[PATCH v2] make pack-objects a bit more resilient to repo corruption

[Nicolas Pitre]

Right now, packing valid objects could fail when creating a thin pack
simply because a pack edge object used as a preferred base is corrupted.
Since preferred base objects are not strictly needed to produce a valid
pack, let's not consider the inability to read them as a fatal error.
Delta compression may well be attempted against other objects in the
search window.  To avoid warning storms (we are in the inner loop of
the delta search window) a warning is emitted only on the first 
occurrence.

Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
---

On Fri, 22 Oct 2010, Nicolas Pitre wrote:

> On Fri, 22 Oct 2010, Jeff King wrote:
> 
> > By converting this die() into a silent return, are we losing a place
> > where git might previously have alerted a user to corruption? In this
> > case, we can continue the operation without the object, but if we have
> > detected corruption, letting the user know as soon as possible is
> > probably a good idea.
> > 
> > In other words, should this instead be:
> > 
> >   warning("unable to read preferred base object: %s", ...);
> >   return 0;
> 
> Well, this get called repeatedly, being within the inner part of the 
> delta search loop.  So you might get that warning as many times as the 
> delta window which is not that nice.  If anything a static flag to 
> display the warning only once would be needed.  But you're pretty likely 
> to have met that warning/error already from other operations, which is 
> why I didn't bother.
> 
> > Or will some other part of the code already complained to stderr?
> 
> Some other part is likely to already have complained, through 
> check_object() -> sha1_object_info().  But not necessarily in all cases.

OK... After further analysis, it seems that the cases when problem 
objects are already warned about through sha1_object_info(), those 
objects will never end up in the delta search window, as their type ends 
up being a negative error code.  We already support that possibility on 
purpose even.

So let's add a warning for when check_object() was able to bypass
the more expensive sha1_object_info() call and therefore object 
corruptions remain undetected until that point.

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f8eba53..81155b4 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1298,9 +1298,23 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 		read_lock();
 		src->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);
 		read_unlock();
-		if (!src->data)
+		if (!src->data) {
+			if (src_entry->preferred_base) {
+				static int warned = 0;
+				if (!warned++)
+					warning("object %s cannot be read",
+						sha1_to_hex(src_entry->idx.sha1));
+				/* 
+				 * Those objects are not included in the
+				 * resulting pack.  Be resilient and ignore
+				 * them if they can't be read, in case the
+				 * pack could be created nevertheless.
+				 */
+				return 0;
+			}
 			die("object %s cannot be read",
 			    sha1_to_hex(src_entry->idx.sha1));
+		}
 		if (sz != src_size)
 			die("object %s inconsistent object length (%lu vs %lu)",
 			    sha1_to_hex(src_entry->idx.sha1), sz, src_size);

Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption

[Sverre Rabbelier]

Heya,

On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre <nico@fluxnic.net> wrote:
> +                               static int warned = 0;
> +                               if (!warned++)
> +                                       warning("object %s cannot be read",
> +                                               sha1_to_hex(src_entry->idx.sha1));

How does this handle multiple missing objects? Will it only warn for
the first one?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 943 bytes --]

On Fri, 22 Oct 2010, Sverre Rabbelier wrote:

> Heya,
> 
> On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre <nico@fluxnic.net> wrote:
> > +                               static int warned = 0;
> > +                               if (!warned++)
> > +                                       warning("object %s cannot be read",
> > +                                               sha1_to_hex(src_entry->idx.sha1));
> 
> How does this handle multiple missing objects? Will it only warn for
> the first one?

Yes, only the first one, so you have a bone to chase if that ever 
happens to you.  And that's good enough IMHO.  Trying to warn for every 
missing object would require extra storage per object to remember if any 
particular object was warned for already, which is I think overkill for 
an extremely unlikely event.  Comprehensive reporting is the job of 
fsck.


Nicolas

Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

> Yes, only the first one, ...
> ...  Comprehensive reporting is the job of 
> fsck.

I had the same knee-jerk reaction, and about to suggest using one bit from
flags, but I agree the design balance you struck here is a good one.

Thanks.

Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption

[Geert Bosch]

On Oct 22, 2010, at 17:19, Nicolas Pitre wrote:

>> On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre <nico@fluxnic.net> wrote:
>>> +                               static int warned = 0;
>>> +                               if (!warned++)
>>> +                                       warning("object %s cannot be read",
>>> +                                               sha1_to_hex(src_entry->idx.sha1));
>> 
>> How does this handle multiple missing objects? Will it only warn for
>> the first one?
> 
> Yes, only the first one, so you have a bone to chase if that ever 
> happens to you.  And that's good enough IMHO.  Trying to warn for every 
> missing object would require extra storage per object to remember if any 
> particular object was warned for already, which is I think overkill for 
> an extremely unlikely event.  Comprehensive reporting is the job of 
> fsck.

Maybe add a ", run git fsck" to the message. Will still comfortably fit a line.

  -Geert

Re: [PATCH v2] make pack-objects a bit more resilient to repo corruption

[Nicolas Pitre]

On Sat, 23 Oct 2010, Geert Bosch wrote:

> 
> On Oct 22, 2010, at 17:19, Nicolas Pitre wrote:
> 
> >> On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre <nico@fluxnic.net> wrote:
> >>> +                               static int warned = 0;
> >>> +                               if (!warned++)
> >>> +                                       warning("object %s cannot be read",
> >>> +                                               sha1_to_hex(src_entry->idx.sha1));
> >> 
> >> How does this handle multiple missing objects? Will it only warn for
> >> the first one?
> > 
> > Yes, only the first one, so you have a bone to chase if that ever 
> > happens to you.  And that's good enough IMHO.  Trying to warn for every 
> > missing object would require extra storage per object to remember if any 
> > particular object was warned for already, which is I think overkill for 
> > an extremely unlikely event.  Comprehensive reporting is the job of 
> > fsck.
> 
> Maybe add a ", run git fsck" to the message. Will still comfortably fit a line.

Maybe if this message ever gets printed often enough.  Let's see if 
someone will even report it before 2012, and be clueless about it. And 
to be consistent, you'd have to do the same throughout the code where 
this could be relevant.

Furthermore, if someone really need the additional clue, then I'm afraid 
that the current fsck output won't help at all except to confuse that 
person even more.

Better for people to ask for help on this list when things break due to 
corruptions if they can't figure it out on their own.


Nicolas