Copilot commented on code in PR #3284:
URL: https://github.com/apache/dubbo-go/pull/3284#discussion_r3044121632
##########
proxy/proxy_factory/utils.go:
##########
@@ -46,7 +55,11 @@ func callLocalMethod(method reflect.Method, in
[]reflect.Value) ([]reflect.Value
}
}()
- returnValues = method.Func.Call(in)
+ if useCallSlice {
+ returnValues = method.Func.CallSlice(in)
+ } else {
+ returnValues = method.Func.Call(in)
+ }
Review Comment:
`reflect.Value.CallSlice` requires the final argument to be a **slice** (not
an array). Allowing `reflect.Array` here can still panic at runtime.
Recommendation: restrict the `useCallSlice` condition to `reflect.Slice` only,
or explicitly convert an array final arg to a slice (`v.Slice(0, v.Len())`)
before calling `CallSlice`.
##########
proxy/proxy_factory/utils.go:
##########
@@ -26,12 +26,21 @@ import (
perrors "github.com/pkg/errors"
)
-// CallLocalMethod is used to handle invoke exception in user func.
+// callLocalMethod invokes method with the given arguments, recovering from
panics.
+// For variadic methods where the last argument is already a typed slice, it
uses
+// CallSlice to correctly expand the slice into variadic parameters.
func callLocalMethod(method reflect.Method, in []reflect.Value)
([]reflect.Value, error) {
var (
returnValues []reflect.Value
retErr error
)
+ useCallSlice := false
+ if method.Type.IsVariadic() && len(in) == method.Type.NumIn() {
+ lastIdx := len(in) - 1
+ if in[lastIdx].IsValid() && (in[lastIdx].Kind() ==
reflect.Slice || in[lastIdx].Kind() == reflect.Array) {
Review Comment:
`reflect.Value.CallSlice` requires the final argument to be a **slice** (not
an array). Allowing `reflect.Array` here can still panic at runtime.
Recommendation: restrict the `useCallSlice` condition to `reflect.Slice` only,
or explicitly convert an array final arg to a slice (`v.Slice(0, v.Len())`)
before calling `CallSlice`.
```suggestion
if in[lastIdx].IsValid() && in[lastIdx].Kind() == reflect.Slice
{
```
##########
filter/generic/service_filter.go:
##########
@@ -78,44 +80,126 @@ func (f *genericServiceFilter) Invoke(ctx context.Context,
invoker base.Invoker,
`, mtdName, types, args)
// get the type of the argument
- ivkUrl := invoker.GetURL()
- svc := common.ServiceMap.GetServiceByServiceKey(ivkUrl.Protocol,
ivkUrl.ServiceKey())
+ ivkURL := invoker.GetURL()
+ svc := common.ServiceMap.GetServiceByServiceKey(ivkURL.Protocol,
ivkURL.ServiceKey())
method := svc.Method()[mtdName]
if method == nil {
return &result.RPCResult{
- Err: perrors.Errorf("\"%s\" method is not found,
service key: %s", mtdName, ivkUrl.ServiceKey()),
+ Err: perrors.Errorf("\"%s\" method is not found,
service key: %s", mtdName, ivkURL.ServiceKey()),
}
}
+
argsType := method.ArgsType()
+ if err := validateGenericArgs(method.IsVariadic(), len(argsType),
len(args), mtdName); err != nil {
+ return &result.RPCResult{Err: err}
+ }
// get generic info from attachments of invocation, the default value
is "true"
generic := inv.GetAttachmentWithDefaultValue(constant.GenericKey,
constant.GenericSerializationDefault)
// get generalizer according to value in the `generic`
- g := getGeneralizer(generic)
+ // realize
+ newArgs, err := realizeInvocationArgs(getGeneralizer(generic),
argsType, args, method.IsVariadic())
+ if err != nil {
+ return &result.RPCResult{Err: err}
+ }
- if len(args) != len(argsType) {
- return &result.RPCResult{
- Err: perrors.Errorf("the number of args(=%d) is not
matched with \"%s\" method", len(args), mtdName),
+ newIvc := invocation.NewRPCInvocation(mtdName, newArgs,
inv.Attachments())
+ newIvc.SetReply(inv.Reply())
+
+ return invoker.Invoke(ctx, newIvc)
+}
+
+// validateGenericArgs checks whether the number of generic invocation
arguments
+// matches the target method signature. Variadic methods accept argCount >=
fixedParams.
+func validateGenericArgs(isVariadic bool, argsTypeCount, argCount int,
methodName string) error {
+ if isVariadic {
+ if argCount >= argsTypeCount-1 {
+ return nil
}
+ } else if argCount == argsTypeCount {
+ return nil
}
- // realize
+ return perrors.Errorf("the number of args(=%d) is not matched with
\"%s\" method", argCount, methodName)
+}
+
+// realizeInvocationArgs converts generic invocation arguments to concrete
types.
+// For variadic methods, fixed params are realized normally and trailing args
are
+// reshaped into a typed slice via realizeVariadicArg.
+func realizeInvocationArgs(g generalizer.Generalizer, argsType []reflect.Type,
args []hessian.Object, isVariadic bool) ([]any, error) {
+ if !isVariadic {
+ return realizeFixedArgs(g, args, argsType)
+ }
+
+ newArgs, err := realizeFixedArgs(g, args[:len(argsType)-1],
argsType[:len(argsType)-1])
+ if err != nil {
+ return nil, err
+ }
+
+ variadicArg, err := realizeVariadicArg(g, args[len(argsType)-1:],
argsType[len(argsType)-1])
+ if err != nil {
+ return nil, err
+ }
+
+ return append(newArgs, variadicArg), nil
+}
+
+// realizeFixedArgs converts each arg to its corresponding concrete type via
Generalizer.Realize.
+func realizeFixedArgs(g generalizer.Generalizer, args []hessian.Object,
argsType []reflect.Type) ([]any, error) {
newArgs := make([]any, len(argsType))
- for i := 0; i < len(argsType); i++ {
+ for i := range argsType {
newArg, err := g.Realize(args[i], argsType[i])
if err != nil {
- return &result.RPCResult{
- Err: perrors.Errorf("realization failed, %v",
err),
- }
+ return nil, perrors.Errorf("realization failed, %v",
err)
}
newArgs[i] = newArg
}
- // build a normal invocation
- newIvc := invocation.NewRPCInvocation(mtdName, newArgs,
inv.Attachments())
- newIvc.SetReply(inv.Reply())
+ return newArgs, nil
+}
- return invoker.Invoke(ctx, newIvc)
+// realizeVariadicArg reshapes trailing generic args into a typed slice (e.g.
[]string)
+// for the variadic parameter. Handles both discrete args and single packed
array from Java.
+func realizeVariadicArg(g generalizer.Generalizer, args []hessian.Object,
variadicSliceType reflect.Type) (any, error) {
+ variadicArgs := normalizeVariadicArgs(args)
+ slice := reflect.MakeSlice(variadicSliceType, len(variadicArgs),
len(variadicArgs))
+ elemType := variadicSliceType.Elem()
+
+ for i, arg := range variadicArgs {
+ realized, err := g.Realize(arg, elemType)
+ if err != nil {
+ return nil, perrors.Errorf("realization of variadic
arg[%d] failed: %v", i, err)
+ }
+ slice.Index(i).Set(reflect.ValueOf(realized))
Review Comment:
`slice.Index(i).Set(reflect.ValueOf(realized))` can panic if `realized` is
nil (invalid `reflect.Value`) or if `realized` is
convertible-but-not-assignable to `elemType`. Recommendation: defensively
handle nil by using `reflect.Zero(elemType)`, and when non-nil prefer
`AssignableTo` / `ConvertibleTo` checks (convert when needed) before calling
`Set`, so malformed or edge-case realizations don’t crash the invocation.
```suggestion
dst := slice.Index(i)
if realized == nil {
dst.Set(reflect.Zero(elemType))
continue
}
realizedValue := reflect.ValueOf(realized)
realizedType := realizedValue.Type()
switch {
case realizedType.AssignableTo(elemType):
dst.Set(realizedValue)
case realizedType.ConvertibleTo(elemType):
dst.Set(realizedValue.Convert(elemType))
default:
return nil, perrors.Errorf(
"realization of variadic arg[%d] produced %v,
cannot assign to %v",
i, realizedType, elemType,
)
}
```
##########
proxy/proxy_factory/default.go:
##########
@@ -130,7 +130,24 @@ func (pi *ProxyInvoker) Invoke(ctx context.Context,
invocation base.Invocation)
}
// prepare argv
- if (len(method.ArgsType()) == 1 || len(method.ArgsType()) == 2 &&
method.ReplyType() == nil) && method.ArgsType()[0].String() == "[]interface {}"
{
+ // NOTE: variadic check must come before the []interface{} passthrough
check below,
+ // because a variadic method like func(args ...interface{}) also has
argsType[0] == []interface{},
+ // and the passthrough branch would incorrectly wrap the pre-packed
slice in another layer.
+ if method.IsVariadic() && len(args) == len(method.ArgsType()) {
+ // Variadic method with pre-packed slice arg (e.g. from generic
filter).
+ // The last arg is already a typed slice — pass it directly so
CallSlice can handle it.
+ for i := 0; i < len(args); i++ {
+ t := reflect.ValueOf(args[i])
+ if !t.IsValid() {
+ at := method.ArgsType()[i]
+ if at.Kind() == reflect.Ptr {
+ at = at.Elem()
+ }
+ t = reflect.New(at)
+ }
+ in = append(in, t)
+ }
+ } else if (len(method.ArgsType()) == 1 || len(method.ArgsType()) == 2
&& method.ReplyType() == nil) && method.ArgsType()[0].String() == "[]interface
{}" {
Review Comment:
When `args[i]` is nil/invalid, this branch builds a value with
`reflect.New(at)` after *possibly stripping pointer-ness via `at = at.Elem()`*.
That produces a `*T` even when the method parameter type is `T` (or produces
the wrong pointer level), which can cause reflection call panics due to type
mismatch. Recommendation: create the correct zero value for the **original**
parameter type: for non-pointer params use `reflect.Zero(at)` (or
`reflect.New(at).Elem()`), for pointer params use `reflect.New(at.Elem())`, and
for slice params (notably the variadic slice) consider `reflect.MakeSlice(at,
0, 0)` to represent “no variadic args”.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
