clang UBSan fixes

[seanm]

This fixes almost all UBSan warnings emitted when running NetCDF's own unit tests, and all such warnings running VTK's unit tests.

[seanm]

@WardF @DennisHeimbigner friendly ping...

[WardF]

Taking a look, thanks @seanm !

[seanm]

@WardF great, thanks. Once these simple fixes are in, I'll create Issues for the remaining tests that fail under UBSan.

[WardF]

Seeing a test failure under Visual Studio on Windows, but not immediately clear what the source is; investigating further.

[WardF]

Absent the Visual Studio failure, how does it look to you @DennisHeimbigner ?

[seanm]

Where do you see the VS failure? It says "All checks have passed. 98 successful checks" for me...

[WardF]

@seanm Not all of the Visual Studio testing has been automated (unfortunately) at this point; I'm seeing it on my local Windows dev machine.

[DennisHeimbigner]

This would probably be better to check for dst being NULL and throwing an error.

[DennisHeimbigner]

Same comment. It is better to explicitly check for NULL and either throw error
or avoid doing the subsequent code with NULL, which will eventually fail.

[seanm]

I just want to be sure you understand what this change, and the similar ones, is doing...

int
NC_calcsize(const NC3_INFO *ncp, off_t *calcsizep)
{
	NC_var **vpp = (NC_var **)ncp->vars.value;

-	NC_var *const *const end = &vpp[ncp->vars.nelems];
+	NC_var *const *const end = vpp ? &vpp[ncp->vars.nelems] : NULL;

First, all these things were not found by static analysis, but by dynamic analysis. That is to say, all the new checks for NULL are because NULL actually occurs (via an existing test case). So there's no hypotheticals here.

In the above case, vpp can be null. I saw it happen. When it is, end is set to NULL + some small offset. In other words, end is pointing to garbage. Now, this is not soooo bad because the code was already careful to not dereference end when vpp is null. So in some sense there was no bug here!

But UBSan is very pedantic. There mere act of creating a pointer to garbage, even if you don't dereference it, is undefined behaviour. So these changes just avoid making such a pointer, and using NULL instead. As before, the pointer that was garbage (and is now NULL) is still not dereferenced.

[DennisHeimbigner]

What makes you think the data is not aligned?

[seanm]

I observed it at runtime. (UBSan is dynamic analysis.)

[DennisHeimbigner]

I made a few comments then stopped because they are repetitive.
I am leery of using NULL as the value for a copy (in various forms).
I think it is better to test explicitly for NULL and then either throw an error
or explicitly bypass the subsequent code that might fail if the target dest. is NULL.

[DennisHeimbigner]

BTW you have identified some potential bugs; thank you for that.
My only concern is that the fixes you propose are not the best solution.

[seanm]

BTW you have identified some potential bugs; thank you for that. My only concern is that the fixes you propose are not the best solution.

I am certainly open to better solutions. I don't know the NetCDF codebase. I only use it via other libraries in fact. I am trying to get all my unit tests passing under UBSan, and the only issues left are all in 3rd party open source libraries.

So concretely, sticking with NC_calcsize: if vpp is NULL, what should the function do?

[seanm]

@DennisHeimbigner friendly ping... can I help move this along?

[WardF]

Rerunning checks and refreshing myself on this so that I can help move it along.

[seanm]

@WardF @DennisHeimbigner friendly re-ping...

It seems you didn't like my solution, started adding other commits here, but not sure where we are now...

(I'd really love to see these solved, they are blocking my work fixing UBSan issues in other projects that use NetCDF, because so much of what remains in them is due to these.)

[DennisHeimbigner]

I modified some of the proposed fixed to be consistent with our coding style. It is just that there are a lot of them to fix.

[WardF]

Fixed conflict, waiting for tests to finish before approving & merging.

[seanm]

@WardF I've tried to review your changes, but it's confusing with my commits still there, then commits changing something, then more commits changing the same lines. Are you going to squash and rebase? I think that would make review easier...

[seanm]

@WardF it's been 5 months now, is there anything I can do to help move this along? Maybe I could split this into smaller PRs and try to follow your preferred style more closely?

[WardF]

Thanks for the ping; pushed an empty commit to re-run the tests.

[seanm]

@WardF since this is clearly not working... let's try more digestible pieces.

Here's a first one: #2787

[seanm]

I'm closing this in favour of making smaller PRs.

The first, #2787, is now merged. The second is ready: #2800

I'll make more once those are in.