Skip to content

Commit 23c2187

Browse files
committed
Fixed Router#Find panic an infinite loop
Before this fix, Router#Find panics or enters in an infinite loop when the context params values were set to a number less than the max number of params supported by the Router.
1 parent 151ed6b commit 23c2187

File tree

3 files changed

+93
-2
lines changed

3 files changed

+93
-2
lines changed

context.go

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -310,15 +310,35 @@ func (c *context) ParamNames() []string {
310310

311311
func (c *context) SetParamNames(names ...string) {
312312
c.pnames = names
313-
*c.echo.maxParam = len(names)
313+
314+
l := len(names)
315+
if *c.echo.maxParam < l {
316+
*c.echo.maxParam = l
317+
}
318+
319+
if len(c.pvalues) < l {
320+
// Keeping the old pvalues just for backward compatibility, but it sounds that doesn't make sense to keep them,
321+
// probably those values will be overriden in a Context#SetParamValues
322+
newPvalues := make([]string, l)
323+
copy(newPvalues, c.pvalues)
324+
c.pvalues = newPvalues
325+
}
314326
}
315327

316328
func (c *context) ParamValues() []string {
317329
return c.pvalues[:len(c.pnames)]
318330
}
319331

320332
func (c *context) SetParamValues(values ...string) {
321-
c.pvalues = values
333+
// NOTE: Don't just set c.pvalues = values, because it has to have length c.echo.maxParam at all times
334+
// It will brake the Router#Find code
335+
limit := len(values)
336+
if limit > *c.echo.maxParam {
337+
limit = *c.echo.maxParam
338+
}
339+
for i := 0; i < limit; i++ {
340+
c.pvalues[i] = values[i]
341+
}
322342
}
323343

324344
func (c *context) QueryParam(name string) string {

context_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,40 @@ func TestContextGetAndSetParam(t *testing.T) {
508508
})
509509
}
510510

511+
// Issue #1655
512+
func TestContextSetParamNamesShouldUpdateEchoMaxParam(t *testing.T) {
513+
assert := testify.New(t)
514+
515+
e := New()
516+
assert.Equal(0, *e.maxParam)
517+
518+
expectedOneParam := []string{"one"}
519+
expectedTwoParams := []string{"one", "two"}
520+
expectedThreeParams := []string{"one", "two", ""}
521+
expectedABCParams := []string{"A", "B", "C"}
522+
523+
c := e.NewContext(nil, nil)
524+
c.SetParamNames("1", "2")
525+
c.SetParamValues(expectedTwoParams...)
526+
assert.Equal(2, *e.maxParam)
527+
assert.EqualValues(expectedTwoParams, c.ParamValues())
528+
529+
c.SetParamNames("1")
530+
assert.Equal(2, *e.maxParam)
531+
// Here for backward compatibility the ParamValues remains as they are
532+
assert.EqualValues(expectedOneParam, c.ParamValues())
533+
534+
c.SetParamNames("1", "2", "3")
535+
assert.Equal(3, *e.maxParam)
536+
// Here for backward compatibility the ParamValues remains as they are, but the len is extended to e.maxParam
537+
assert.EqualValues(expectedThreeParams, c.ParamValues())
538+
539+
c.SetParamValues("A", "B", "C", "D")
540+
assert.Equal(3, *e.maxParam)
541+
// Here D shouldn't be returned
542+
assert.EqualValues(expectedABCParams, c.ParamValues())
543+
}
544+
511545
func TestContextFormValue(t *testing.T) {
512546
f := make(url.Values)
513547
f.Set("name", "Jon Snow")

router_test.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1298,6 +1298,43 @@ func TestRouterParam1466(t *testing.T) {
12981298
assert.Equal(t, 0, c.response.Status)
12991299
}
13001300

1301+
// Issue #1655
1302+
func TestRouterFindNotPanicOrLoopsWhenContextSetParamValuesIsCalledWithLessValuesThanEchoMaxParam(t *testing.T) {
1303+
e := New()
1304+
r := e.router
1305+
1306+
v0 := e.Group("/:version")
1307+
v0.GET("/admin", func(c Context) error {
1308+
c.SetParamNames("version")
1309+
c.SetParamValues("v1")
1310+
return nil
1311+
})
1312+
1313+
v0.GET("/images/view/:id", handlerHelper("iv", 1))
1314+
v0.GET("/images/:id", handlerHelper("i", 1))
1315+
v0.GET("/view/*", handlerHelper("v", 1))
1316+
1317+
//If this API is called before the next two one panic the other loops ( of course without my fix ;) )
1318+
c := e.NewContext(nil, nil)
1319+
r.Find(http.MethodGet, "/v1/admin", c)
1320+
c.Handler()(c)
1321+
assert.Equal(t, "v1", c.Param("version"))
1322+
1323+
//panic
1324+
c = e.NewContext(nil, nil)
1325+
r.Find(http.MethodGet, "/v1/view/same-data", c)
1326+
c.Handler()(c)
1327+
assert.Equal(t, "same-data", c.Param("*"))
1328+
assert.Equal(t, 1, c.Get("v"))
1329+
1330+
//looping
1331+
c = e.NewContext(nil, nil)
1332+
r.Find(http.MethodGet, "/v1/images/view", c)
1333+
c.Handler()(c)
1334+
assert.Equal(t, "view", c.Param("id"))
1335+
assert.Equal(t, 1, c.Get("i"))
1336+
}
1337+
13011338
func benchmarkRouterRoutes(b *testing.B, routes []*Route) {
13021339
e := New()
13031340
r := e.router

0 commit comments

Comments
 (0)