CA client crashes on providing 0 as user callback

[Ralph Lange]

When calling ca_array_put_callback() (or one of its cousins) with 0 as user callback, the client segfaults on the first "execution" of that user callback. I.e. the callback is not checked against being NULL before added to the subscription context.

IMHO the CA client should be a bit more robust and not add the user callback when it is NULL.

[Andrew Johnson (anj)]

Agreed that this should not cause the client to crash. Whether it should be an error though is an interesting question.

For ca_array_get_callback() and ca_create_subscription() I can see no valid reason to want to pass a NULL callback routine, so those routines should return an error immediately in that case. In fact looking at the code I see that ca_create_subscription() is already doing that check and returns ECA_BADFUNCPTR if pCallBack is NULL.

For ca_array_put_callback() though a client might want to trigger the dbNotify mechanism in the IOC but not get told the result. For the purposes of this bug-fix I'm not proposing we allow that without some discussion, but we might consider adding it later.

The attached patch to the 3.14 branch adds checks in 2 places: Return ECA_BADFUNCPTR from the C API implementation, and also prevents the callback mechanisms from trying to call a NULL function pointer (since the internal C++ API does not do the checks). Not tested.

[Ralph Lange (ralph-lange)]

I was not considering to make supplying NULL return an error for the *_put_callback() variants.
Until now it was not returning an error, and I would like to see justification for a behavior change.

Instead, the code in oldChannelNotify.cpp could probably be simplified by making *_put() macros that call *_put_callback() supplying NULL as the callback.

[Andrew Johnson (anj)]

If I've understood you correctly, that *would* cause a behavior change because ca_array_put_callback() triggers the dbNotify mechanism on the IOC whereas ca_array_put() does not. The CAref documentation says nothing about what is supposed to happen if pfunc is NULL.

I don't know what if anything actually changed, I suspect it may be that our current C++ compilers no longer bother to check for a NULL function pointer before calling it because that would be Undefined Behaviour, and if you trigger that you're on your own. Just because it used to work if you passed a NULL doesn't mean it was legal even then.

As I said before, I see no point in allowing a NULL callback function to ca_array_get_callback(), so I think that should return an error. For ca_array_put_callback() I could go either way, but if we allow it I think it should still trigger the dbNotify mechanism in the IOC — I'm pretty sure it used to do that, so to keep the original behavior we just have to remove the check that my patch adds to that function and rely on the new test in putCallback.cpp instead.

[Ralph Lange (ralph-lange)]

Ah - sorry, I was thinking in knots, distracted, offset, and stupid.

What I meant was :
Instead, the code in oldChannelNotify.cpp could probably be simplified by adding a new method that takes an additional flag triggering the notify on the IOC, and making *_put() and *_put_callback() macros that call the new method supplying the pointer or NULL as the callback and setting the flag argument.
Looking more closely, this might not simplify too much. With the additional obfuscating macro layer, it's probably a draw.

My main concern was that adding a putCallback structure on the client with a NULL pointer, which is bound to segfault the client when the notify completes, does not look convincing.

With respect to regarding a NULL pointer as an error for put_callback(), I think we are free to decide.
It was not returning an error in the past, but it was reliably crashing the client. I think we can safely assume no one was using a NULL pointer as a feature.
I also see no use case for using put_callback() without a callback. For consistency, I would make NULL an error, as with get_callback().

I like your patch - maybe also remove the unnecessary (* ) in getCallback.cpp as you did in putCallback.cpp

[Andrew Johnson (anj)]

Committed to 3.14, thanks. I will merge 3.14 fixes and start on the branch merges into 3.15 next week, things are a bit hectic here this week.