Patchwork [STABLE] templater: protect word() from crashing on out of range negative value

login
register
mail settings
Submitter Matt Harbison
Date Oct. 5, 2015, 4:45 p.m.
Message ID <05972c639f5fdb09048d.1444063543@waste.org>
Download mbox | patch
Permalink /patch/10811/
State Accepted
Headers show

Comments

Matt Harbison - Oct. 5, 2015, 4:45 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1444063046 14400
#      Mon Oct 05 12:37:26 2015 -0400
# Branch stable
# Node ID 05972c639f5fdb09048d2f4de2397ba29ce8427a
# Parent  93bfa9fc96e31f1cc5f444bdc2436966c665cf1f
templater: protect word() from crashing on out of range negative value

The function isn't documented to work with negative values at all, but it does,
which can be useful.  However, the range check didn't account for this.
Pierre-Yves David - Oct. 6, 2015, 10:43 p.m.
On 10/05/2015 09:45 AM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1444063046 14400
> #      Mon Oct 05 12:37:26 2015 -0400
> # Branch stable
> # Node ID 05972c639f5fdb09048d2f4de2397ba29ce8427a
> # Parent  93bfa9fc96e31f1cc5f444bdc2436966c665cf1f
> templater: protect word() from crashing on out of range negative value
>
> The function isn't documented to work with negative values at all, but it does,
> which can be useful.  However, the range check didn't account for this.

Sweet, pushed to the clowncopter.
Yuya Nishihara - Oct. 7, 2015, 12:22 p.m.
On Tue, 06 Oct 2015 15:43:26 -0700, Pierre-Yves David wrote:
> On 10/05/2015 09:45 AM, Matt Harbison wrote:
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1444063046 14400
> > #      Mon Oct 05 12:37:26 2015 -0400
> > # Branch stable
> > # Node ID 05972c639f5fdb09048d2f4de2397ba29ce8427a
> > # Parent  93bfa9fc96e31f1cc5f444bdc2436966c665cf1f
> > templater: protect word() from crashing on out of range negative value
> >
> > The function isn't documented to work with negative values at all, but it does,
> > which can be useful.  However, the range check didn't account for this.

> > -    if num >= len(tokens):
> > +    if abs(num) >= len(tokens):

Off-by-one error? Negative index should be inclusive.

I think catching IndexError would be less error-prone.
Pierre-Yves David - Oct. 7, 2015, 7:05 p.m.
On 10/07/2015 05:22 AM, Yuya Nishihara wrote:
> On Tue, 06 Oct 2015 15:43:26 -0700, Pierre-Yves David wrote:
>> On 10/05/2015 09:45 AM, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1444063046 14400
>>> #      Mon Oct 05 12:37:26 2015 -0400
>>> # Branch stable
>>> # Node ID 05972c639f5fdb09048d2f4de2397ba29ce8427a
>>> # Parent  93bfa9fc96e31f1cc5f444bdc2436966c665cf1f
>>> templater: protect word() from crashing on out of range negative value
>>>
>>> The function isn't documented to work with negative values at all, but it does,
>>> which can be useful.  However, the range check didn't account for this.
>
>>> -    if num >= len(tokens):
>>> +    if abs(num) >= len(tokens):
>
> Off-by-one error? Negative index should be inclusive.
>
> I think catching IndexError would be less error-prone.

snap, +1 for IndexError
Matt Mackall - Oct. 7, 2015, 7:15 p.m.
On Wed, 2015-10-07 at 12:05 -0700, Pierre-Yves David wrote:
> 
> On 10/07/2015 05:22 AM, Yuya Nishihara wrote:
> > On Tue, 06 Oct 2015 15:43:26 -0700, Pierre-Yves David wrote:
> >> On 10/05/2015 09:45 AM, Matt Harbison wrote:
> >>> # HG changeset patch
> >>> # User Matt Harbison <matt_harbison@yahoo.com>
> >>> # Date 1444063046 14400
> >>> #      Mon Oct 05 12:37:26 2015 -0400
> >>> # Branch stable
> >>> # Node ID 05972c639f5fdb09048d2f4de2397ba29ce8427a
> >>> # Parent  93bfa9fc96e31f1cc5f444bdc2436966c665cf1f
> >>> templater: protect word() from crashing on out of range negative value
> >>>
> >>> The function isn't documented to work with negative values at all, but it does,
> >>> which can be useful.  However, the range check didn't account for this.
> >
> >>> -    if num >= len(tokens):
> >>> +    if abs(num) >= len(tokens):
> >
> > Off-by-one error? Negative index should be inclusive.
> >
> > I think catching IndexError would be less error-prone.
> 
> snap, +1 for IndexError

Already fixed in flight to actually check the right range.

Patch

diff --git a/mercurial/templater.py b/mercurial/templater.py
--- a/mercurial/templater.py
+++ b/mercurial/templater.py
@@ -649,7 +649,7 @@ 
         splitter = None
 
     tokens = text.split(splitter)
-    if num >= len(tokens):
+    if abs(num) >= len(tokens):
         return ''
     else:
         return tokens[num]
diff --git a/tests/test-command-template.t b/tests/test-command-template.t
--- a/tests/test-command-template.t
+++ b/tests/test-command-template.t
@@ -3368,6 +3368,11 @@ 
   hg: parse error: word expects an integer index
   [255]
 
+Test word for out of range
+
+  $ hg log -R a --template "{word(10000, desc)}"
+  $ hg log -R a --template "{word(-10000, desc)}"
+
 Test indent and not adding to empty lines
 
   $ hg log -T "-----\n{indent(desc, '>> ', ' > ')}\n" -r 0:1 -R a