code review 6931049: regexp: fix index panic in Replace

regexp: fix index panic in Replace

When using subexpressions ($1) as replacements, when they either don't exist or values weren't found causes a panic.
This patch ensures that the match location isn't -1, to prevent out of bounds errors.
Fixes issue 3816.

[erikstmartin, 2012-12-12 02:52:57]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[fss, 2012-12-12 03:00:07]

https://codereview.appspot.com/6931049/diff/5001/src/pkg/regexp/all_test.go
File src/pkg/regexp/all_test.go (right):

https://codereview.appspot.com/6931049/diff/5001/src/pkg/regexp/all_test.go#n...
src/pkg/regexp/all_test.go:200: // Subtitution when subexpression isn't found
s/Subtitution/Substitution/

[erikstmartin, 2012-12-12 03:03:50]

oops, typo. Let me update that. Thanks :)

On Tuesday, December 11, 2012 10:00:07 PM UTC-5, Francisco Souza wrote:
>
>
> https://codereview.appspot.com/6931049/diff/5001/src/pkg/regexp/all_test.go 
> File src/pkg/regexp/all_test.go (right): 
>
>
https://codereview.appspot.com/6931049/diff/5001/src/pkg/regexp/all_test.go#n...

>
>
src/pkg/regexp/all_test.go:200<https://codereview.appspot.com/6931049/diff/5001/src/pkg/regexp/all_test.go#newcode200src/pkg/regexp/all_test.go:200>:

> // Subtitution when subexpression isn't 
> found 
> s/Subtitution/Substitution/ 
>
> https://codereview.appspot.com/6931049/ 
>

[erikstmartin, 2012-12-12 03:05:22]

Hello golang-dev@googlegroups.com, franciscossouza@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-12-12 04:17:39]

looks good thanks
not sure what the ... in the CL desc is for

regexp: fix index panic in Replace

https://codereview.appspot.com/6931049/diff/2002/src/pkg/regexp/regexp.go
File src/pkg/regexp/regexp.go (right):

https://codereview.appspot.com/6931049/diff/2002/src/pkg/regexp/regexp.go#new...
src/pkg/regexp/regexp.go:770: if 2*num+1 < len(match) && match[2*num+1] >= 0 {
use 2*num in the new phrase instead of 2*num+1, to match same condition below
(it's 2*i below)

[erikstmartin, 2012-12-12 04:26:13]

I think it's just truncating the description because it's too long, and adding
an ellipsis in place of the rest of the description.

On 2012/12/12 04:17:39, rsc wrote:
> looks good thanks
> not sure what the ... in the CL desc is for
> 
> regexp: fix index panic in Replace
> 
> https://codereview.appspot.com/6931049/diff/2002/src/pkg/regexp/regexp.go
> File src/pkg/regexp/regexp.go (right):
> 
>
https://codereview.appspot.com/6931049/diff/2002/src/pkg/regexp/regexp.go#new...
> src/pkg/regexp/regexp.go:770: if 2*num+1 < len(match) && match[2*num+1] >= 0 {
> use 2*num in the new phrase instead of 2*num+1, to match same condition below
> (it's 2*i below)

[rsc, 2012-12-12 19:25:10]

Oh, right. Either way, please make the first line just:

regexp: fix index panic in Replace

[erikstmartin, 2012-12-12 19:28:26]

On 2012/12/12 19:25:10, rsc wrote:
> Oh, right. Either way, please make the first line just:
> 
> regexp: fix index panic in Replace

Done.

[erikstmartin, 2012-12-19 15:20:17]

On 2012/12/12 19:28:26, erikstmartin wrote:
> On 2012/12/12 19:25:10, rsc wrote:
> > Oh, right. Either way, please make the first line just:
> > 
> > regexp: fix index panic in Replace
> 
> Done.

PTAL

[rsc, 2012-12-22 15:14:51]

It looks like you need to run 'hg upload 6931049'.

[erikstmartin, 2012-12-22 15:36:40]

On 2012/12/22 15:14:51, rsc wrote:
> It looks like you need to run 'hg upload 6931049'.

hg upload has been run, PTAL

[rsc, 2012-12-22 15:41:55]

https://codereview.appspot.com/6931049/diff/2002/src/pkg/regexp/regexp.go
File src/pkg/regexp/regexp.go (right):

https://codereview.appspot.com/6931049/diff/2002/src/pkg/regexp/regexp.go#new...
src/pkg/regexp/regexp.go:770: if 2*num+1 < len(match) && match[2*num+1] >= 0 {
On 2012/12/12 04:17:40, rsc wrote:
> use 2*num in the new phrase instead of 2*num+1, to match same condition below
> (it's 2*i below)

This comment appears to have been missed.

[erikstmartin, 2012-12-22 15:57:41]

On 2012/12/22 15:41:55, rsc wrote:
> https://codereview.appspot.com/6931049/diff/2002/src/pkg/regexp/regexp.go
> File src/pkg/regexp/regexp.go (right):
> 
>
https://codereview.appspot.com/6931049/diff/2002/src/pkg/regexp/regexp.go#new...
> src/pkg/regexp/regexp.go:770: if 2*num+1 < len(match) && match[2*num+1] >= 0 {
> On 2012/12/12 04:17:40, rsc wrote:
> > use 2*num in the new phrase instead of 2*num+1, to match same condition
below
> > (it's 2*i below)
> 
> This comment appears to have been missed.

You're right, i did miss that comment. I've updated the phrase as suggested.

[rsc, 2012-12-22 16:14:59]

*** Submitted as https://code.google.com/p/go/source/detail?r=2414f5fb898d ***

      regexp: fix index panic in Replace

When using subexpressions ($1) as replacements, when they either don't exist or
values weren't found causes a panic.
This patch ensures that the match location isn't -1, to prevent out of bounds
errors.
Fixes issue 3816.

R=franciscossouza, rsc
CC=golang-dev
https://codereview.appspot.com/6931049

Committer: Russ Cox <rsc@golang.org>

[rsc, 2012-12-22 16:15:33]

LGTM

[remyoudompheng, 2013-07-20 20:01:06]

R=close