code scanning alert in generated code

[dhivyasa]

The generated mock code trigged off by one error in a code scanning utility, codeQL

https://github.com/github/codeql/blob/78fcbd07d654881d9d3395efc0ea371c392529de/go/ql/src/InconsistentCode/LengthComparisonOffByOn

	params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations)
	if len(params) > 0 {

The check for len of params triggered it. Not sure if it is a false positive or there is abetter way to do this other than using indexes.
This happened on Tracer package. Also I think all generated code follow similar pattern
pegomock generate"go.opentelemetry.io/otel/trace" Tracer

[petergtz]

Hi @dhivyasa, thanks for reporting this. I'll have a closer look at this tomorrow. I'm almost sure this is a false positive. I don't even quite get what the scanner tries to flag 🙂. Again, I'll have a closer look tomorrow.

[petergtz]

@dhivyasa I had a closer look at this, specifically trying to figure out what codeQL is doing here. I must admit, it's not clear why it would flag this as an off-by-one. The examples do not relate to the actual code you pasted here and the code is also correct in what it's doing. Maybe CodeQL gets somehow confused by the fact that this is generated code and for that reason there are a lot of hardcoded indices.

Does this answer your question?

[petergtz]

Closing due to inactivity. Please re-open, if needed.

[AndrewRPorter]

Hey @petergtz 👋, I am running into this as well.

Take the following generated code snippet for example:

func (c *MockService_MethodName_OngoingVerification) GetAllCapturedArguments() (_param0 []context.Context, _param1 []*models.ModelName) {
	params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations)
	if len(params) > 0 {
Dismissed
		_param0 = make([]context.Context, len(c.methodInvocations))
		for u, param := range params[0] {
			_param0[u] = param.(context.Context)
		}
		_param1 = make([]*models.ModelName, len(c.methodInvocations))
		for u, param := range params[1] {
			_param1[u] = param.(*models.ModelName)
		}
	}
	return
}

I believe the CodeQL error is complaining since technically the > 0 check can pass if the length is 1 but then our params[1] would be an out of bounds access.

[petergtz]

Thanks for shedding light on this @AndrewRPorter. I understand it now. I think CodeQL is still wrong and the code is correct. Do you happen to know if we can add a suppression comment to avoid this false positive?

Alternatively, we could change the comparison to len(params) != 0 assuming this would satisfy CodeQL. The result of len can never be negative, so the logic is effectively equivalent.

Fancy submitting a PR?

[AndrewRPorter]

Do you happen to know if we can add a suppression comment to avoid this false positive?

I am able to suppress the alert as "used in tests" but for some larger mocks this can be an annoying processing suppressing many alerts.

I think CodeQL is still wrong and the code is correct.

Can you help me understand why you think this? I see the following cases,

• len(params) = 0
  • In this case we just return
• len(params) = 1
  • CodeQL I think it complainnig about this case due to the out of bound access. Here I would expect a check for len(params) > 1 instead to fix the CodeQL error
• len(params) > 1
  • This case is fine

---

I'm happy to help contribute a fix

[petergtz]

I am able to suppress the alert as "used in tests" but for some larger mocks this can be an annoying processing suppressing many alerts.

What I meant here, was: make a code change in Pegomock to generate code that tells CodeQL to ignore this. Python's flake8 linter (and others allows this via # noqa.

Can you help me understand why you think this?

There are only 2 cases:

• params is empty --> len(params) == 0 --> don't do anything
• params is not empty --> len(params) != 0 or len(params) > 0--> execute the generated code. The code that has generated this code knows how many items are in params through other means and therefore is able to use "hard-coded" indexes. You can see it in

pegomock/mockgen/mockgen.go

Lines 352 to 373 in dcebf66

	g.p("if len(_params) > 0 {")
	for i, argType := range argTypes {
	if isVariadic && i == len(argTypes)-1 {
	variadicBasicType := strings.Replace(argType, "[]", "", 1)
	g.
	p("_param%v = make([]%v, len(c.methodInvocations))", i, argType).
	p("for u := 0; u < len(c.methodInvocations); u++ {").
	p("_param%v[u] = make([]%v, len(_params)-%v)", i, variadicBasicType, i).
	p("for x := %v; x < len(_params); x++ {", i).
	p("if _params[x][u] != nil {").
	p("_param%v[u][x-%v] = _params[x][u].(%v)", i, i, variadicBasicType).
	p("}").
	p("}").
	p("}")
	break
	} else {
	g.p("_param%v = make([]%v, len(c.methodInvocations))", i, argType)
	g.p("for u, param := range _params[%v] {", i)
	g.p("_param%v[u]=param.(%v)", i, argType)
	g.p("}")
	}
	}

That knowledge cannot be seen in the generated code and therefore CodeQL cannot know it. CodeQL here is really not useful.

[AndrewRPorter]

Thanks for explaining! Unfortunately it looks like CodeQL does not support inline suppressions right now: github/codeql#11427.

[petergtz]

Addressed by #126. Closing this issue.