ENHANCE: Avoid UBSAN errors

[mgondan]

See here: https://www.stats.ox.ac.uk/pub/bdr/memtests/clang-UBSAN/rswipl/rswipl.log

The individual errors are mentioned in the individual commits. Four error messages are still left, these require inspection, I'll open an issue.

[mgondan]

The above mentioned issue is here, #1290

[mgondan]

I think the remaining problems have been solved with the last two commits.

[JanWielemaker]

Hmm. Except for pl-unzip.c, most of this makes the code uglier without giving a lot of benefits. We could probably avoid some/all of the offset warnings by using the standard offsetof(), which was not standard when most of this was implemented. I'd welcome that. Do we really want to avoid the refs->preallocated - 1 index-out-of-bound? Yes, it is out of bound, but that is still the case after decrementing it and it is the whole point of these thread-safe dynamically scaling arrays.

[mgondan]

Do we really want to avoid the
refs->preallocated - 1;
index-out-of-bound?

I agree that some of the workarounds are not pretty, but having code that passes UB checks is a good thing in itself.

And I mean,

p = refs->preallocated; p--;

is not that ugly, or is it?

[mgondan]

Updated the comment after having pressed "submit" too quickly.

[JanWielemaker]

And I mean,

p = refs->preallocated; p--;

is not that ugly, or is it?

Well, it is hard to explain and I'd probably change the code back when scanning the code later ... More seriously, it is just as "buggy", but just hides it a level deeper such that the current tools do not spot it. The code uses pointers outside an array and the code ensures these pointers used with an offset that gets/sets valid data. That is just a step too complicated for these tools.

The question to me is what are we trying to improve and/or who are we trying to satisfy? If that is sufficiently motivated I vote for some macro that captures this assignment which we can document. That avoids people asking themselves what this code is doing.

[mgondan]

More seriously, it is just as "buggy", but just hides it a level deeper such that the current tools do not spot it.

Good point indeed.

The question to me is what are we trying to improve and/or who are we trying to satisfy?

The usual motivation, the R interface ("rswipl"/"rolog"). They remove the package from CRAN if it's not fixed.

[JanWielemaker]

They remove the package from CRAN if it's not fixed.

That is a good reason 😢 Do you think you can fix the offset issues using standard offsetof()? I'm happy to have a look whether I can find something more elegant to this.

How do I reproduce this? I thought the code was ubsan-safe after the previous series of changes?

[mgondan]

Reproduce with: CC="clang -fsanitize=undefined" cmake -DSWIPL_PACKAGES_X=OFF -DINSTALL_DOCUMENTATION=OFF ..

Unsure if clang is needed, may also work with gcc

I thought the code was ubsan-safe after the previous series of changes?

IIRC the previous fixes referred to rswipl's own routines. Now it's swipl's own build process.

[mgondan]

Do you think you can fix the offset issues using standard offsetof()?

I can have a look at it next week

[mgondan]

Do you think you can fix the offset issues using standard offsetof()?

Done. It's a bit less ugly now.

[JanWielemaker]

All the work in this PR is handled. clang-18 seems to add some more ubsan issues, among which checking the types for calls over function pointers. This raised a couple of possible portability issues. I fixed some of them, but not yet all. I now have to work on other projects ....

Thanks for the work.