Fix #781, Terminate UT macro variadic lists

[skliper]

Describe the contribution
Fix #781 - terminates the variadic lists with NULL in the unit test macros

Testing performed
Build and execute unit tests, pass

Expected behavior changes
Avoids CodeQL warning

System(s) tested on

• Hardware: cFS Dev Server
• OS: Ubuntu 18.04
• Versions: cFS Bundle main + this commit

Additional context
None

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

[jphickey]

I'm not so sure about this one. The point of the variable arguments here is to basically pass the original function arguments to the hook function. The extra NULL might cause problems, as it wasn't in the original set. Arguments are not restricted to pointers, and no hook functions should be relying on a NULL argument to terminate the va arg list (args depends on the function being stubbed out).

The real intent of varadic args here is to allow a hook function of a printf-style function to be registered one and then still pass the args to e.g. v[sn]printf() as needed -- but this use-case would be invoking UT_DefaultStubImpl directly, not using the macro, so I guess it wouldn't break that....

Maybe its OK, but I'm not sure if there might be unintended consequences of adding a NULL pointer that wasn't part of the original function args.

[jphickey]

Better question - why does CodeQL think a variadic argument list needs to be terminated with some sentinel value? That is more of the exception than the rule.

[skliper]

We can ignore the warnings and retract this PR if there's an example/scenario it breaks.

[jphickey]

Does it actually need to break? I would ignore the warning just based on correctness i.e. it is more correct the way it is, this makes it less correct. The variadic args are supposed to match the original set. If you add a NULL it violates this, and no longer matches the original set. So its arguably wrong, even if it it might not break the current use cases, it might break a future one.

It is also impossible to actually use this NULL sentinel, because previous args are not necessarily pointers. So as far as I can tell its only to squelch a codeQL warning, and provides zero benefit to actual code safety (and a negative impact on correctness).

This is definitely a warning I would recommend turning off, if possible. I'm not sure what the justification of this one is.

[jphickey]

This reminds me of previous discussions over the years of how we should respond to "issues" found by tools. Some reports are great and they find stuff that is otherwise missed. Some are not so great, and some so-called rules even conflict with eachother (IIRC MISRA does this, in some circumstances, you are supposed to choose one based on local policy).

We should not bend over backward to "fix" every issue by the code checking tool of the day. If it provides benefit, then great. But some "rules" seem to be implemented just for the sake of finding issues, and don't really improve things.

[skliper]

But these macros are used for non-variadic functions. Is the NULL in the va list really less "correct" than an empty va list when we are dealing with non-variadic functions? Either way it should be getting ignored so why not just resolve and avoid seeing the warning in every repo that uses this and show up for any customer who uses a similar check?

osal/ut_assert/src/utstubs.c

Lines 822 to 829 in 09a2c5e

	if (StubPtr->Data.Cb.IsVarg)
	{
	Retcode = (*StubPtr->Data.Cb.Ptr.Va)(StubPtr->Data.Cb.CallbackArg, Retcode, Counter, ContextPtr, va);
	}
	else
	{
	Retcode = (*StubPtr->Data.Cb.Ptr.Simple)(StubPtr->Data.Cb.CallbackArg, Retcode, Counter, ContextPtr);
	}

[jphickey]

But these macros are used for non-variadic functions. Is the NULL in the va list really less "correct" than an empty va list when we are dealing with non-variadic functions? Either way it should be getting ignored so why not just resolve and avoid seeing the warning in every repo that uses this and show up for any customer who uses a similar check?

The macro converts a non variadic function into a variadic one, using whatever arguments were supplied. This allows a standardized hook prototype to be used while passing any number of args. The va_args are supposed to be interpreted by the hook by the value in the UT_FuncKey_t (this takes the role of the spec string in a printf()-style, or the flags to open(), etc).

There is no part of this that says it should have an extra NULL pointer at the end, and furthermore there is no part that even says the args are pointers to being with. Doing so is wrong, and implies things that shouldn't even be implied (i.e. that someone could loop through args). If someone tried to do this they would inevitably end up casting the args as something they are not (pointers), which is undefined behavior. That is (at least IMO) a worse/bigger risk than simply leaving it as-is.

As I said before, a while a variadic function that accepts an arbitrary number of same-type args is possible, and DOES need a terminator, it isn't what we have here.

To make a analogy, this is basically saying "your uint32 is missing its NULL terminator" (as if everything was a char string).

Sure, you can add a null terminator to your integers.... but why? IMO "because codeQL" isn't sufficient - tomorrow there will be another checking tool that might flag it the other way.

[jphickey]

BTW- out of curiosity, how come code QL is complaining about this one, but not OS_printf() and the like? We don't put an extra NULL argument at the end of every printf argument set. It is the same thing.

[skliper]

Still not convinced. I can see the argument for supplying a known value in the variadic list when used with non-variadic functions. Seems like a general hook could utilize this. You could have a hook that checks for the NULL pointer or loops through the va list. Why limit user hooks if we don't need to? The contract could just state this behavior and I'd think we'd be good.

[skliper]

While I can't think of an actual use case, I don't see why we need to make it invalid to use a general UT_VaHookFunc_t with stubs that may or may not have a va list. Ending it w/ a known value just seems to make sense in this case. And if you hook w/ a UT_HookFunc_t then it gets ignored... nothing broken.

[skliper]

It may be smart enough to tell that the va list use isn't bounded, since it gets passed into the hook? For the other cases maybe it can tell the implementation doesn't depend on termination.

[jphickey]

For the other cases maybe it can tell the implementation doesn't depend on termination.

This is most use cases - and the more typical one - were a variadic function relies on another (fixed) parameter, to identify what the variadic part has.

My point was - since codeQL isn't flagging these functions as unterminated argument lists, maybe there is a way (via a comment or something?) to tell it that the arguments are determined by the FuncKey here, and its not open-ended.

My guess is, that most likely the checker doesn't know that the Key is serving to identify the arguments to the hook routine, so it is assuming its open-ended. But that assumption is wrong. Making an incorrect/inappropriate code change to squelch an invalid warning based on an incorrect assumption isn't ideal. Better would be to tell the tool what we are doing so it doesn't make incorrect assumptions.

I can see the argument for supplying a known value in the variadic list when used with non-variadic functions. Seems like a general hook could utilize this

I can't see either. It makes no sense, and I can't see how a "general hook" could use it. Using a sentinel/terminator like this assumes that all args are the same type, but they are not. Its a broken (and dangerous!) assumption. Shouldn't do it.

[skliper]

I see it this way, if you add a hook using UT_HookFunc_t va is ignored. If you add a hook using UT_VaHookFunc_t to a stub that has no va list, having a terminated list could be useful to generalize the stub if there's a "class" of functions you want to test (with and without a variable sized list). UT_VaHookFunc_t wouldn't have to handle every different possible key, it could just process the list until the termination.

You are forcing users to use the key, why require this restriction (when it also squashes a static analysis warning)? I don't see this implementation as incorrect in any way, it just provides a termination if the user wants to leverage that behavior.

EDIT - I definitely wouldn't argue for a change if it didn't also squash the warning. I just don't agree the NULL termination is something that's really wrong in this case.

[jphickey]

NULL termination here is incorrect/wrong because you can only do this if the variadic function meets two important criteria:

1. All args are always known to be pointers.
2. a NULL pointer is never possible / valid.

So, for instance, it works if everything is a string (as in the execl() and friends in POSIX). But neither is true here.

One cannot check a pointer for NULL until one knows that it is a pointer to begin with. When processing a va_list one must first know the type of the arg - not the other way around.

We should never imply that one can write a hook function that works with multiple different functions without looking at the key value. (except for cases where functions have the exact same set of args, or the hook doesn't even need to look at the args).

One cannot ever interpret a va_list without knowing what type the args are supposed to be - and this is done via one of the fixed arguments. The "key" in this case. That's just how variadic functions work.

All I can say here is that "CodeQL" is a tool - not some sort of authority. This is clearly an example where "fixing" one of its warnings would make the code a little more dubious/risky - the opposite of what it is supposed to do. While I concur it won't break anything currently in there - it implies something that simply isn't correct/valid so I don't like it at all.

[skliper]

I disagree, what I keep hearing is your personal vision for how these utilities can be used vs any reference to a coding standard or other hard requirement for behavior. Probably need mediation via @astrogeco @jwilmot @acudmore. Adding the NULL in the macro for a non-variadic function and defining that as what it does (therefore all parties KNOW what it's supposed to be for this case) violates no rule I'm aware of. In fact I think it is a benefit per my previous statements.

[skliper]

Why require looking at the key from the hook? The user knows what they are registering for, if the patterns are general enough it's easy to write a hook that could be used with a class of stubs...

[skliper]

Your claim that "neither is true here" is unsubstantiated. Both can be true if the user wants to define their hook and APIs that way. You are forcing the user into patterns that are not necessary to enforce. They can and should be able to choose this perfectly valid pattern of a va list of pointers where a null terminates the list.

Obviously this wouldn't work for a stub that does have a real va list (which wouldn't use these macro's anyways) or for a va hook that doesn't meet this pattern (for which a user shouldn't register it on a stub that doesn't have a va list), or they even could use the key to do the anti-pattern for the case if they want to use the hook with a stub does have a va list that doesn't meet the criteria above. Using the key (to know the stub doesn't have a va list and the macro will pass in a NULL that should be ignored) seems to solve even this fairly non-realistic case without breakage.

[skliper]

@jphickey updated per the tag-up, happy to adjust phrasing if needed.