Patchwork [STABLE] repoview: fix corrupted hiddencache crash Mercurial (issue5042)

login
register
mail settings
Submitter Laurent Charignon
Date Jan. 20, 2016, 6:09 p.m.
Message ID <688f61fa0e0a8cc9139a.1453313367@dev5073.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12844/
State Superseded
Commit 97e0dc6d248c2e0dbf14747a7b88ee2a3cefee67
Headers show

Comments

Laurent Charignon - Jan. 20, 2016, 6:09 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1453313317 28800
#      Wed Jan 20 10:08:37 2016 -0800
# Branch stable
# Node ID 688f61fa0e0a8cc9139a533cb3fe6cb411b1dc8f
# Parent  3203dfe341f962e33256d6475fc3585563db78ad
repoview: fix corrupted hiddencache crash Mercurial (issue5042)

Before this patch if the hiddencache existed but was empty, it would crash
mercurial. This patch adds exception handling when reading the hiddencache to
avoid the issue.
When encountering a corrupted cache file we print a devel warning. There would
be no point in issuing a normal warning as the user wouldn't be able to do
anything about the situation.

The warning looks like:

devel-warn: corrupted hidden cache, removing it at: /path/to/repoview.py
Gregory Szorc - Jan. 20, 2016, 6:18 p.m.
> On Jan 20, 2016, at 10:09, Laurent Charignon <lcharignon@fb.com> wrote:
> 
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1453313317 28800
> #      Wed Jan 20 10:08:37 2016 -0800
> # Branch stable
> # Node ID 688f61fa0e0a8cc9139a533cb3fe6cb411b1dc8f
> # Parent  3203dfe341f962e33256d6475fc3585563db78ad
> repoview: fix corrupted hiddencache crash Mercurial (issue5042)
> 
> Before this patch if the hiddencache existed but was empty, it would crash
> mercurial. This patch adds exception handling when reading the hiddencache to
> avoid the issue.
> When encountering a corrupted cache file we print a devel warning. There would
> be no point in issuing a normal warning as the user wouldn't be able to do
> anything about the situation.
> 
> The warning looks like:
> 
> devel-warn: corrupted hidden cache, removing it at: /path/to/repoview.py
> 
> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> --- a/mercurial/repoview.py
> +++ b/mercurial/repoview.py
> @@ -150,6 +150,12 @@
>                 count = len(data) / 4
>                 hidden = frozenset(struct.unpack('>%ii' % count, data))
>         return hidden
> +    except struct.error:
> +        repo.ui.develwarn('corrupted hidden cache, removing it')
> +        if fh:
> +            fh.close()
> +        repo.vfs.unlink(cachefile)
> +        return hidden

I don't have the full source in front of me. Do we handle read errors the same way? Do we have test coverage for e.g. permissions failure on file reading? If not, we should add it.

>     finally:
>         if fh:
>             fh.close()
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -951,6 +951,11 @@
>   $ hg amendtransient
>   [1, 3]
> 
> +Check that corrupted hidden cache does not crash hg
> +  $ printf "" > .hg/cache/hidden
> +  $ hg status
> +  devel-warn: corrupted hidden cache, removing it at: * (glob)
> +
> Test cache consistency for the visible filter
> 1) We want to make sure that the cached filtered revs are invalidated when
> bookmarks change
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Laurent Charignon - Jan. 20, 2016, 8:46 p.m.
> On Jan 20, 2016, at 10:18 AM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
> 
> 
>> On Jan 20, 2016, at 10:09, Laurent Charignon <lcharignon@fb.com> wrote:
>> 
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon@fb.com>
>> # Date 1453313317 28800
>> #      Wed Jan 20 10:08:37 2016 -0800
>> # Branch stable
>> # Node ID 688f61fa0e0a8cc9139a533cb3fe6cb411b1dc8f
>> # Parent  3203dfe341f962e33256d6475fc3585563db78ad
>> repoview: fix corrupted hiddencache crash Mercurial (issue5042)
>> 
>> Before this patch if the hiddencache existed but was empty, it would crash
>> mercurial. This patch adds exception handling when reading the hiddencache to
>> avoid the issue.
>> When encountering a corrupted cache file we print a devel warning. There would
>> be no point in issuing a normal warning as the user wouldn't be able to do
>> anything about the situation.
>> 
>> The warning looks like:
>> 
>> devel-warn: corrupted hidden cache, removing it at: /path/to/repoview.py
>> 
>> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
>> --- a/mercurial/repoview.py
>> +++ b/mercurial/repoview.py
>> @@ -150,6 +150,12 @@
>>                count = len(data) / 4
>>                hidden = frozenset(struct.unpack('>%ii' % count, data))
>>        return hidden
>> +    except struct.error:
>> +        repo.ui.develwarn('corrupted hidden cache, removing it')
>> +        if fh:
>> +            fh.close()
>> +        repo.vfs.unlink(cachefile)
>> +        return hidden
> 
> I don't have the full source in front of me. Do we handle read errors the same way? Do we have test coverage for e.g. permissions failure on file reading? If not, we should add it.


What you mention is already handled at a lower level and we should probably add test for it in another patch.

$ chmod 000 .hg/cache/hidden
$ hg st
abort: Permission denied: /home/lcharignon/hg/.hg/cache/hidden

Thanks,

Lauremt

> 
>>    finally:
>>        if fh:
>>            fh.close()
>> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
>> --- a/tests/test-obsolete.t
>> +++ b/tests/test-obsolete.t
>> @@ -951,6 +951,11 @@
>>  $ hg amendtransient
>>  [1, 3]
>> 
>> +Check that corrupted hidden cache does not crash hg
>> +  $ printf "" > .hg/cache/hidden
>> +  $ hg status
>> +  devel-warn: corrupted hidden cache, removing it at: * (glob)
>> +
>> Test cache consistency for the visible filter
>> 1) We want to make sure that the cached filtered revs are invalidated when
>> bookmarks change
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> https://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Jan. 20, 2016, 8:59 p.m.
On Wed, 2016-01-20 at 20:46 +0000, Laurent Charignon wrote:
> > On Jan 20, 2016, at 10:18 AM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> > 
> > 
> > 
> > > On Jan 20, 2016, at 10:09, Laurent Charignon <lcharignon@fb.com> wrote:
> > > 
> > > # HG changeset patch
> > > # User Laurent Charignon <lcharignon@fb.com>
> > > # Date 1453313317 28800
> > > #      Wed Jan 20 10:08:37 2016 -0800
> > > # Branch stable
> > > # Node ID 688f61fa0e0a8cc9139a533cb3fe6cb411b1dc8f
> > > # Parent  3203dfe341f962e33256d6475fc3585563db78ad
> > > repoview: fix corrupted hiddencache crash Mercurial (issue5042)
> > > 
> > > Before this patch if the hiddencache existed but was empty, it would crash
> > > mercurial. This patch adds exception handling when reading the hiddencache
> > > to
> > > avoid the issue.
> > > When encountering a corrupted cache file we print a devel warning. There
> > > would
> > > be no point in issuing a normal warning as the user wouldn't be able to do
> > > anything about the situation.
> > > 
> > > The warning looks like:
> > > 
> > > devel-warn: corrupted hidden cache, removing it at: /path/to/repoview.py
> > > 
> > > diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> > > --- a/mercurial/repoview.py
> > > +++ b/mercurial/repoview.py
> > > @@ -150,6 +150,12 @@
> > >                count = len(data) / 4
> > >                hidden = frozenset(struct.unpack('>%ii' % count, data))
> > >        return hidden
> > > +    except struct.error:
> > > +        repo.ui.develwarn('corrupted hidden cache, removing it')
> > > +        if fh:
> > > +            fh.close()
> > > +        repo.vfs.unlink(cachefile)
> > > +        return hidden
> > 
> > I don't have the full source in front of me. Do we handle read errors the
> > same way? Do we have test coverage for e.g. permissions failure on file
> > reading? If not, we should add it.

I think deleting the file is not the right policy. We should overwrite it (if we
have permission) when refreshing the cache, but otherwise silently ignore it.

> What you mention is already handled at a lower level and we should probably
> add test for it in another patch.
> 
> $ chmod 000 .hg/cache/hidden
> $ hg st
> abort: Permission denied: /home/lcharignon/hg/.hg/cache/hidden

This should also be silently ignored.

-- 
Mathematics is the supreme nostalgia of our time.
Laurent Charignon - Jan. 20, 2016, 9:39 p.m.
> On Jan 20, 2016, at 12:59 PM, Matt Mackall <mpm@selenic.com> wrote:
> 
> On Wed, 2016-01-20 at 20:46 +0000, Laurent Charignon wrote:
>>> On Jan 20, 2016, at 10:18 AM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>>> 
>>> 
>>> 
>>>> On Jan 20, 2016, at 10:09, Laurent Charignon <lcharignon@fb.com> wrote:
>>>> 
>>>> # HG changeset patch
>>>> # User Laurent Charignon <lcharignon@fb.com>
>>>> # Date 1453313317 28800
>>>> #      Wed Jan 20 10:08:37 2016 -0800
>>>> # Branch stable
>>>> # Node ID 688f61fa0e0a8cc9139a533cb3fe6cb411b1dc8f
>>>> # Parent  3203dfe341f962e33256d6475fc3585563db78ad
>>>> repoview: fix corrupted hiddencache crash Mercurial (issue5042)
>>>> 
>>>> Before this patch if the hiddencache existed but was empty, it would crash
>>>> mercurial. This patch adds exception handling when reading the hiddencache
>>>> to
>>>> avoid the issue.
>>>> When encountering a corrupted cache file we print a devel warning. There
>>>> would
>>>> be no point in issuing a normal warning as the user wouldn't be able to do
>>>> anything about the situation.
>>>> 
>>>> The warning looks like:
>>>> 
>>>> devel-warn: corrupted hidden cache, removing it at: /path/to/repoview.py
>>>> 
>>>> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
>>>> --- a/mercurial/repoview.py
>>>> +++ b/mercurial/repoview.py
>>>> @@ -150,6 +150,12 @@
>>>>                count = len(data) / 4
>>>>                hidden = frozenset(struct.unpack('>%ii' % count, data))
>>>>        return hidden
>>>> +    except struct.error:
>>>> +        repo.ui.develwarn('corrupted hidden cache, removing it')
>>>> +        if fh:
>>>> +            fh.close()
>>>> +        repo.vfs.unlink(cachefile)
>>>> +        return hidden
>>> 
>>> I don't have the full source in front of me. Do we handle read errors the
>>> same way? Do we have test coverage for e.g. permissions failure on file
>>> reading? If not, we should add it.
> 
> I think deleting the file is not the right policy. We should overwrite it (if we
> have permission) when refreshing the cache, but otherwise silently ignore it.

The file gets rewritten correctly right after we compute "hidden" in that case as we return None.
Let's silently ignore it, I will send a V2.

> 
>> What you mention is already handled at a lower level and we should probably
>> add test for it in another patch.
>> 
>> $ chmod 000 .hg/cache/hidden
>> $ hg st
>> abort: Permission denied: /home/lcharignon/hg/.hg/cache/hidden
> 
> This should also be silently ignored.

Sure, I will add code to silence the error and print it only in debug mode.

Thanks,

Laurent

> 
> -- 
> Mathematics is the supreme nostalgia of our time.

Patch

diff --git a/mercurial/repoview.py b/mercurial/repoview.py
--- a/mercurial/repoview.py
+++ b/mercurial/repoview.py
@@ -150,6 +150,12 @@ 
                 count = len(data) / 4
                 hidden = frozenset(struct.unpack('>%ii' % count, data))
         return hidden
+    except struct.error:
+        repo.ui.develwarn('corrupted hidden cache, removing it')
+        if fh:
+            fh.close()
+        repo.vfs.unlink(cachefile)
+        return hidden
     finally:
         if fh:
             fh.close()
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -951,6 +951,11 @@ 
   $ hg amendtransient
   [1, 3]
 
+Check that corrupted hidden cache does not crash hg
+  $ printf "" > .hg/cache/hidden
+  $ hg status
+  devel-warn: corrupted hidden cache, removing it at: * (glob)
+
 Test cache consistency for the visible filter
 1) We want to make sure that the cached filtered revs are invalidated when
 bookmarks change