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