Merge pull request #1661 from pafuent/fix_router_find_after_invalid_set_param_values

Fixed Router#Find panic an infinite loop

Author: lammel

context.go

@@ -314,15 +314,35 @@ func (c *context) ParamNames() []string {
 
 func (c *context) SetParamNames(names ...string) {
 	c.pnames = names
-	*c.echo.maxParam = len(names)
+
+	l := len(names)
+	if *c.echo.maxParam < l {
+		*c.echo.maxParam = l
+	}
+
+	if len(c.pvalues) < l {
+		// Keeping the old pvalues just for backward compatibility, but it sounds that doesn't make sense to keep them,
+		// probably those values will be overriden in a Context#SetParamValues
+		newPvalues := make([]string, l)
+		copy(newPvalues, c.pvalues)
+		c.pvalues = newPvalues
+	}
 }
 
 func (c *context) ParamValues() []string {
 	return c.pvalues[:len(c.pnames)]
 }
 
 func (c *context) SetParamValues(values ...string) {
-	c.pvalues = values
+	// NOTE: Don't just set c.pvalues = values, because it has to have length c.echo.maxParam at all times
+	// It will brake the Router#Find code
+	limit := len(values)
+	if limit > *c.echo.maxParam {
+		limit = *c.echo.maxParam
+	}
+	for i := 0; i < limit; i++ {
+		c.pvalues[i] = values[i]
+	}
 }
 
 func (c *context) QueryParam(name string) string {

context_test.go

@@ -517,6 +517,40 @@ func TestContextGetAndSetParam(t *testing.T) {
 	})
 }
 
+// Issue #1655
+func TestContextSetParamNamesShouldUpdateEchoMaxParam(t *testing.T) {
+	assert := testify.New(t)
+
+	e := New()
+	assert.Equal(0, *e.maxParam)
+
+	expectedOneParam := []string{"one"}
+	expectedTwoParams := []string{"one", "two"}
+	expectedThreeParams := []string{"one", "two", ""}
+	expectedABCParams := []string{"A", "B", "C"}
+
+	c := e.NewContext(nil, nil)
+	c.SetParamNames("1", "2")
+	c.SetParamValues(expectedTwoParams...)
+	assert.Equal(2, *e.maxParam)
+	assert.EqualValues(expectedTwoParams, c.ParamValues())
+
+	c.SetParamNames("1")
+	assert.Equal(2, *e.maxParam)
+	// Here for backward compatibility the ParamValues remains as they are
+	assert.EqualValues(expectedOneParam, c.ParamValues())
+
+	c.SetParamNames("1", "2", "3")
+	assert.Equal(3, *e.maxParam)
+	// Here for backward compatibility the ParamValues remains as they are, but the len is extended to e.maxParam
+	assert.EqualValues(expectedThreeParams, c.ParamValues())
+
+	c.SetParamValues("A", "B", "C", "D")
+	assert.Equal(3, *e.maxParam)
+	// Here D shouldn't be returned
+	assert.EqualValues(expectedABCParams, c.ParamValues())
+}
+
 func TestContextFormValue(t *testing.T) {
 	f := make(url.Values)
 	f.Set("name", "Jon Snow")

router_test.go

@@ -1430,6 +1430,43 @@ func TestRouterParam1466(t *testing.T) {
 	assert.Equal(t, 0, c.response.Status)
 }
 
+// Issue #1655
+func TestRouterFindNotPanicOrLoopsWhenContextSetParamValuesIsCalledWithLessValuesThanEchoMaxParam(t *testing.T) {
+	e := New()
+	r := e.router
+
+	v0 := e.Group("/:version")
+	v0.GET("/admin", func(c Context) error {
+		c.SetParamNames("version")
+		c.SetParamValues("v1")
+		return nil
+	})
+
+	v0.GET("/images/view/:id", handlerHelper("iv", 1))
+	v0.GET("/images/:id", handlerHelper("i", 1))
+	v0.GET("/view/*", handlerHelper("v", 1))
+
+	//If this API is called before the next two one panic the other loops ( of course without my fix ;) )
+	c := e.NewContext(nil, nil)
+	r.Find(http.MethodGet, "/v1/admin", c)
+	c.Handler()(c)
+	assert.Equal(t, "v1", c.Param("version"))
+
+	//panic
+	c = e.NewContext(nil, nil)
+	r.Find(http.MethodGet, "/v1/view/same-data", c)
+	c.Handler()(c)
+	assert.Equal(t, "same-data", c.Param("*"))
+	assert.Equal(t, 1, c.Get("v"))
+
+	//looping
+	c = e.NewContext(nil, nil)
+	r.Find(http.MethodGet, "/v1/images/view", c)
+	c.Handler()(c)
+	assert.Equal(t, "view", c.Param("id"))
+	assert.Equal(t, 1, c.Get("i"))
+}
+
 // Issue #1653
 func TestRouterPanicWhenParamNoRootOnlyChildsFailsFind(t *testing.T) {
 	e := New()