Patchwork [2,of,7,stable] bundle1: fix bundle1-denied reporting for push over ssh

login
register
mail settings
Submitter Pierre-Yves David
Date Feb. 10, 2017, 5:53 p.m.
Message ID <f319afe9c93cb0cfeeff.1486749183@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/18406/
State Accepted
Delegated to: Martin von Zweigbergk
Headers show

Comments

Pierre-Yves David - Feb. 10, 2017, 5:53 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1486745819 -3600
#      Fri Feb 10 17:56:59 2017 +0100
# Branch stable
# Node ID f319afe9c93cb0cfeeff58f66b1792eb55130ba4
# Parent  a7ded180ddb35dfc0e642e960a59ed475fd9be75
# EXP-Topic getbundleerror
bundle1: fix bundle1-denied reporting for push over ssh

Changeset b288fb2724bf introduced a config option to have the server deny push
using bundle1. The original protocol has not really be design to allow such kind
of error reporting so some hack was used. It turned the hack only works on HTTP
and that ssh wire peer hangs forever when the same hack is used. After further
digging, there is no way to report the error in a unified way. Using 'ooberror'
freeze ssh and raising 'Abort' makes HTTP return a HTTP500 without further
details. So with sadness we implement a version that dispatch according to the
protocol used.

We also add a test for pushing over ssh to make sure we won't regress in the
future. That test show that the hint is missing, this is another bug fixed in
the next changeset.
via Mercurial-devel - Feb. 10, 2017, 11:54 p.m.
On Fri, Feb 10, 2017 at 9:53 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1486745819 -3600
> #      Fri Feb 10 17:56:59 2017 +0100
> # Branch stable
> # Node ID f319afe9c93cb0cfeeff58f66b1792eb55130ba4
> # Parent  a7ded180ddb35dfc0e642e960a59ed475fd9be75
> # EXP-Topic getbundleerror
> bundle1: fix bundle1-denied reporting for push over ssh
>
> Changeset b288fb2724bf introduced a config option to have the server deny push
> using bundle1. The original protocol has not really be design to allow such kind
> of error reporting so some hack was used. It turned the hack only works on HTTP
> and that ssh wire peer hangs forever when the same hack is used. After further
> digging, there is no way to report the error in a unified way. Using 'ooberror'
> freeze ssh and raising 'Abort' makes HTTP return a HTTP500 without further
> details. So with sadness we implement a version that dispatch according to the
> protocol used.

Could we instead raise a custom exception with the appropriate data (a
main message and a hint, it seems) that we then could catch in the
http and ssh handlers and handle appropriately for each?

Even if that's what we want long-term (I don't know if there's a
reason not want it long-term), I'm fine with taking your patches for
now since they're probably less intrusive and therefore more
appropriate for stable.

>
> We also add a test for pushing over ssh to make sure we won't regress in the
> future. That test show that the hint is missing, this is another bug fixed in
> the next changeset.
>
> diff -r a7ded180ddb3 -r f319afe9c93c mercurial/wireproto.py
> --- a/mercurial/wireproto.py    Fri Feb 10 17:56:47 2017 +0100
> +++ b/mercurial/wireproto.py    Fri Feb 10 17:56:59 2017 +0100
> @@ -33,9 +33,10 @@
>  urlerr = util.urlerr
>  urlreq = util.urlreq
>
> -bundle2required = _(
> -    'incompatible Mercurial client; bundle2 required\n'
> -    '(see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n')
> +bundle2requiredmain = _('incompatible Mercurial client; bundle2 required')
> +bundle2requiredhint = _('see https://www.mercurial-scm.org/wiki/'
> +                        'IncompatibleClient')
> +bundle2required = '%s\n(%s)\n' % (bundle2requiredmain, bundle2requiredhint)
>
>  class abstractserverproto(object):
>      """abstract class that summarizes the protocol API
> @@ -948,7 +949,14 @@
>              gen = exchange.readbundle(repo.ui, fp, None)
>              if (isinstance(gen, changegroupmod.cg1unpacker)
>                  and not bundle1allowed(repo, 'push')):
> -                return ooberror(bundle2required)
> +                if proto.name == 'http':
> +                    # need to special case http because stderr do not get to
> +                    # the http client on failed push so we need to abuse some
> +                    # other error type to make sure the message get to the
> +                    # user.
> +                    return ooberror(bundle2required)
> +                raise error.Abort(bundle2requiredmain,
> +                                  hint=bundle2requiredhint)
>
>              r = exchange.unbundle(repo, gen, their_heads, 'serve',
>                                    proto._client())
> diff -r a7ded180ddb3 -r f319afe9c93c tests/test-bundle2-exchange.t
> --- a/tests/test-bundle2-exchange.t     Fri Feb 10 17:56:47 2017 +0100
> +++ b/tests/test-bundle2-exchange.t     Fri Feb 10 17:56:59 2017 +0100
> @@ -1107,6 +1107,14 @@
>    (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
>    [255]
>
> +(also check with ssh)
> +
> +  $ hg --config devel.legacy.exchange=bundle1 push ssh://user@dummy/bundle2onlyserver
> +  pushing to ssh://user@dummy/bundle2onlyserver
> +  searching for changes
> +  remote: abort: incompatible Mercurial client; bundle2 required
> +  [1]
> +
>    $ hg push
>    pushing to http://localhost:$HGPORT/
>    searching for changes
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Gregory Szorc - Feb. 11, 2017, 6:28 p.m.
> On Feb 10, 2017, at 15:54, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> 
> On Fri, Feb 10, 2017 at 9:53 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1486745819 -3600
>> #      Fri Feb 10 17:56:59 2017 +0100
>> # Branch stable
>> # Node ID f319afe9c93cb0cfeeff58f66b1792eb55130ba4
>> # Parent  a7ded180ddb35dfc0e642e960a59ed475fd9be75
>> # EXP-Topic getbundleerror
>> bundle1: fix bundle1-denied reporting for push over ssh
>> 
>> Changeset b288fb2724bf introduced a config option to have the server deny push
>> using bundle1. The original protocol has not really be design to allow such kind
>> of error reporting so some hack was used. It turned the hack only works on HTTP
>> and that ssh wire peer hangs forever when the same hack is used. After further
>> digging, there is no way to report the error in a unified way. Using 'ooberror'
>> freeze ssh and raising 'Abort' makes HTTP return a HTTP500 without further
>> details. So with sadness we implement a version that dispatch according to the
>> protocol used.
> 
> Could we instead raise a custom exception with the appropriate data (a
> main message and a hint, it seems) that we then could catch in the
> http and ssh handlers and handle appropriately for each?
> 
> Even if that's what we want long-term (I don't know if there's a
> reason not want it long-term), I'm fine with taking your patches for
> now since they're probably less intrusive and therefore more
> appropriate for stable.

Ugh. The SSH wire protocol needs to be replaced.

I agree with Martin about wanting a better error handling mechanism. I also agree that it would be too invasive for stable.

The patches as is correct an obvious problem and LGTM. Refinements can occur on default.

FWIW Mozilla sees a lot of these "stream ended unexpectedly" errors. The errors are next to useless and I really wish we printed something more informational. I still suspect we may be swallowing an exception from a low-level network failure somewhere. What I'm trying to say is there is a lot of room for improvement to error handling in the protocol code. But that's for another series.

> 
>> 
>> We also add a test for pushing over ssh to make sure we won't regress in the
>> future. That test show that the hint is missing, this is another bug fixed in
>> the next changeset.
>> 
>> diff -r a7ded180ddb3 -r f319afe9c93c mercurial/wireproto.py
>> --- a/mercurial/wireproto.py    Fri Feb 10 17:56:47 2017 +0100
>> +++ b/mercurial/wireproto.py    Fri Feb 10 17:56:59 2017 +0100
>> @@ -33,9 +33,10 @@
>> urlerr = util.urlerr
>> urlreq = util.urlreq
>> 
>> -bundle2required = _(
>> -    'incompatible Mercurial client; bundle2 required\n'
>> -    '(see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n')
>> +bundle2requiredmain = _('incompatible Mercurial client; bundle2 required')
>> +bundle2requiredhint = _('see https://www.mercurial-scm.org/wiki/'
>> +                        'IncompatibleClient')
>> +bundle2required = '%s\n(%s)\n' % (bundle2requiredmain, bundle2requiredhint)
>> 
>> class abstractserverproto(object):
>>     """abstract class that summarizes the protocol API
>> @@ -948,7 +949,14 @@
>>             gen = exchange.readbundle(repo.ui, fp, None)
>>             if (isinstance(gen, changegroupmod.cg1unpacker)
>>                 and not bundle1allowed(repo, 'push')):
>> -                return ooberror(bundle2required)
>> +                if proto.name == 'http':
>> +                    # need to special case http because stderr do not get to
>> +                    # the http client on failed push so we need to abuse some
>> +                    # other error type to make sure the message get to the
>> +                    # user.
>> +                    return ooberror(bundle2required)
>> +                raise error.Abort(bundle2requiredmain,
>> +                                  hint=bundle2requiredhint)
>> 
>>             r = exchange.unbundle(repo, gen, their_heads, 'serve',
>>                                   proto._client())
>> diff -r a7ded180ddb3 -r f319afe9c93c tests/test-bundle2-exchange.t
>> --- a/tests/test-bundle2-exchange.t     Fri Feb 10 17:56:47 2017 +0100
>> +++ b/tests/test-bundle2-exchange.t     Fri Feb 10 17:56:59 2017 +0100
>> @@ -1107,6 +1107,14 @@
>>   (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
>>   [255]
>> 
>> +(also check with ssh)
>> +
>> +  $ hg --config devel.legacy.exchange=bundle1 push ssh://user@dummy/bundle2onlyserver
>> +  pushing to ssh://user@dummy/bundle2onlyserver
>> +  searching for changes
>> +  remote: abort: incompatible Mercurial client; bundle2 required
>> +  [1]
>> +
>>   $ hg push
>>   pushing to http://localhost:$HGPORT/
>>   searching for changes
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - Feb. 12, 2017, 7:23 a.m.
On Sat, Feb 11, 2017 at 10:28 AM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>
>
>> On Feb 10, 2017, at 15:54, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>>
>> On Fri, Feb 10, 2017 at 9:53 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>> # Date 1486745819 -3600
>>> #      Fri Feb 10 17:56:59 2017 +0100
>>> # Branch stable
>>> # Node ID f319afe9c93cb0cfeeff58f66b1792eb55130ba4
>>> # Parent  a7ded180ddb35dfc0e642e960a59ed475fd9be75
>>> # EXP-Topic getbundleerror
>>> bundle1: fix bundle1-denied reporting for push over ssh
>>>
>>> Changeset b288fb2724bf introduced a config option to have the server deny push
>>> using bundle1. The original protocol has not really be design to allow such kind
>>> of error reporting so some hack was used. It turned the hack only works on HTTP
>>> and that ssh wire peer hangs forever when the same hack is used. After further
>>> digging, there is no way to report the error in a unified way. Using 'ooberror'
>>> freeze ssh and raising 'Abort' makes HTTP return a HTTP500 without further
>>> details. So with sadness we implement a version that dispatch according to the
>>> protocol used.
>>
>> Could we instead raise a custom exception with the appropriate data (a
>> main message and a hint, it seems) that we then could catch in the
>> http and ssh handlers and handle appropriately for each?
>>
>> Even if that's what we want long-term (I don't know if there's a
>> reason not want it long-term), I'm fine with taking your patches for
>> now since they're probably less intrusive and therefore more
>> appropriate for stable.
>
> Ugh. The SSH wire protocol needs to be replaced.
>
> I agree with Martin about wanting a better error handling mechanism. I also agree that it would be too invasive for stable.
>
> The patches as is correct an obvious problem and LGTM. Refinements can occur on default.
>
> FWIW Mozilla sees a lot of these "stream ended unexpectedly" errors. The errors are next to useless and I really wish we printed something more informational. I still suspect we may be swallowing an exception from a low-level network failure somewhere. What I'm trying to say is there is a lot of room for improvement to error handling in the protocol code. But that's for another series.

Series queued for stable. Thanks to Pierre-Yves for patches and thanks
to Greg for review.

>
>>
>>>
>>> We also add a test for pushing over ssh to make sure we won't regress in the
>>> future. That test show that the hint is missing, this is another bug fixed in
>>> the next changeset.
>>>
>>> diff -r a7ded180ddb3 -r f319afe9c93c mercurial/wireproto.py
>>> --- a/mercurial/wireproto.py    Fri Feb 10 17:56:47 2017 +0100
>>> +++ b/mercurial/wireproto.py    Fri Feb 10 17:56:59 2017 +0100
>>> @@ -33,9 +33,10 @@
>>> urlerr = util.urlerr
>>> urlreq = util.urlreq
>>>
>>> -bundle2required = _(
>>> -    'incompatible Mercurial client; bundle2 required\n'
>>> -    '(see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n')
>>> +bundle2requiredmain = _('incompatible Mercurial client; bundle2 required')
>>> +bundle2requiredhint = _('see https://www.mercurial-scm.org/wiki/'
>>> +                        'IncompatibleClient')
>>> +bundle2required = '%s\n(%s)\n' % (bundle2requiredmain, bundle2requiredhint)
>>>
>>> class abstractserverproto(object):
>>>     """abstract class that summarizes the protocol API
>>> @@ -948,7 +949,14 @@
>>>             gen = exchange.readbundle(repo.ui, fp, None)
>>>             if (isinstance(gen, changegroupmod.cg1unpacker)
>>>                 and not bundle1allowed(repo, 'push')):
>>> -                return ooberror(bundle2required)
>>> +                if proto.name == 'http':
>>> +                    # need to special case http because stderr do not get to
>>> +                    # the http client on failed push so we need to abuse some
>>> +                    # other error type to make sure the message get to the
>>> +                    # user.
>>> +                    return ooberror(bundle2required)
>>> +                raise error.Abort(bundle2requiredmain,
>>> +                                  hint=bundle2requiredhint)
>>>
>>>             r = exchange.unbundle(repo, gen, their_heads, 'serve',
>>>                                   proto._client())
>>> diff -r a7ded180ddb3 -r f319afe9c93c tests/test-bundle2-exchange.t
>>> --- a/tests/test-bundle2-exchange.t     Fri Feb 10 17:56:47 2017 +0100
>>> +++ b/tests/test-bundle2-exchange.t     Fri Feb 10 17:56:59 2017 +0100
>>> @@ -1107,6 +1107,14 @@
>>>   (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
>>>   [255]
>>>
>>> +(also check with ssh)
>>> +
>>> +  $ hg --config devel.legacy.exchange=bundle1 push ssh://user@dummy/bundle2onlyserver
>>> +  pushing to ssh://user@dummy/bundle2onlyserver
>>> +  searching for changes
>>> +  remote: abort: incompatible Mercurial client; bundle2 required
>>> +  [1]
>>> +
>>>   $ hg push
>>>   pushing to http://localhost:$HGPORT/
>>>   searching for changes
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@mercurial-scm.org
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff -r a7ded180ddb3 -r f319afe9c93c mercurial/wireproto.py
--- a/mercurial/wireproto.py	Fri Feb 10 17:56:47 2017 +0100
+++ b/mercurial/wireproto.py	Fri Feb 10 17:56:59 2017 +0100
@@ -33,9 +33,10 @@ 
 urlerr = util.urlerr
 urlreq = util.urlreq
 
-bundle2required = _(
-    'incompatible Mercurial client; bundle2 required\n'
-    '(see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n')
+bundle2requiredmain = _('incompatible Mercurial client; bundle2 required')
+bundle2requiredhint = _('see https://www.mercurial-scm.org/wiki/'
+                        'IncompatibleClient')
+bundle2required = '%s\n(%s)\n' % (bundle2requiredmain, bundle2requiredhint)
 
 class abstractserverproto(object):
     """abstract class that summarizes the protocol API
@@ -948,7 +949,14 @@ 
             gen = exchange.readbundle(repo.ui, fp, None)
             if (isinstance(gen, changegroupmod.cg1unpacker)
                 and not bundle1allowed(repo, 'push')):
-                return ooberror(bundle2required)
+                if proto.name == 'http':
+                    # need to special case http because stderr do not get to
+                    # the http client on failed push so we need to abuse some
+                    # other error type to make sure the message get to the
+                    # user.
+                    return ooberror(bundle2required)
+                raise error.Abort(bundle2requiredmain,
+                                  hint=bundle2requiredhint)
 
             r = exchange.unbundle(repo, gen, their_heads, 'serve',
                                   proto._client())
diff -r a7ded180ddb3 -r f319afe9c93c tests/test-bundle2-exchange.t
--- a/tests/test-bundle2-exchange.t	Fri Feb 10 17:56:47 2017 +0100
+++ b/tests/test-bundle2-exchange.t	Fri Feb 10 17:56:59 2017 +0100
@@ -1107,6 +1107,14 @@ 
   (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
   [255]
 
+(also check with ssh)
+
+  $ hg --config devel.legacy.exchange=bundle1 push ssh://user@dummy/bundle2onlyserver
+  pushing to ssh://user@dummy/bundle2onlyserver
+  searching for changes
+  remote: abort: incompatible Mercurial client; bundle2 required
+  [1]
+
   $ hg push
   pushing to http://localhost:$HGPORT/
   searching for changes