Closes #5510: undefined behaviour

[HughParsonage]

Closes #5510.

As issue #5510 notes, the affected code triggers undefined behaviour. In general this is because when checking whether an integer conversion is possible without losing precision, the following condition is used.

(int)v == v

However, this is undefined behaviour if v cannot be represented as an int. A helper function within_int32_repres is added to the top of utils.c to check this. This function is invoked prior to any use of the above condition. In addition, there was one instance of an attempt to convert nan to int64_t (aka long) which has been protected in gsumm.c to completely close the issue.

A consequence of this was a change to some of the warnings involving a loss of precision in integer64 conversions. I have modified 4 tests to reflect this change, which amounted to changing the warning text to include the possibility the value was "out-of-range".

Some of the tests have been suspended via comments to reflect bugs in the tests themselves uncovered by this PR, and addressed elsewhere. (#5835 at the time of writing)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (92105e8) 97.47% compared to head (6f2769f) 97.47%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5832   +/-   ##
=======================================
  Coverage   97.47%   97.47%           
=======================================
  Files          80       80           
  Lines       14823    14827    +4     
=======================================
+ Hits        14448    14452    +4     
  Misses        375      375           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MichaelChirico]

great find! seems obvious in retrospect... in terms of completeness, have you grepped through to other (int) casts that might be affected but untested?

in terms of future-proofing, any ideas how we might prevent this issue from recurring? feels like there could be a compiler warning

[HughParsonage]

I believe I caught the ones relevant to the issue. I did a brief grep using

hutils::goto_pattern_in("(?<!(f))\\(int\\)", "src", file.ext = ".c")

The ones I saw that might benefit were checked by other, more stringent conditions (e.g. val<0.0). There were others but I could not easily tell whether it was possible for the rvalue to be out-of-range.

---

In terms of future-proofing, all I would do is just be mindful whenever I use (int) that I need to have already directed flow away from the case where it's not possible to convert. It's a similar scenario with arithmetic, where a check for defined behaviour can often itself be the cause of undefined behaviour.

int minus(int a, int b) {
  return a - b; // wrong
}

int minus2(int a, int b) {
  int64_t o = a - b; // still wrong
  return (o >= INT_MIN && o <= INT_MAX) ? o : NA_INTEGER;
}

[MichaelChirico]

needs moar LLM

[tdhock]

hi @HughParsonage please consider volunteering as a reviewer for these files, by adding yourself in CODEOWNERS, so you will be notified if there are any future PRs which affect them.

[HughParsonage]

I've run on rhub with sanitizers and all appears to be ok: https://builder.r-hub.io/status/data.table_1.14.99.tar.gz-4e4196719d354e02850532ae9cf1ad5d

[jangorecki]

I've run on rhub with sanitizers and all appears to be ok: https://builder.r-hub.io/status/data.table_1.14.99.tar.gz-4e4196719d354e02850532ae9cf1ad5d

And running master branch detects those?

[HughParsonage]

I'm struggling to reproduce the UBSAN issues, but in my view this satisfies the undefined behaviour issue identified.

[jangorecki]

Before merging this would be nice to reproduce issues on master and reproduce no issues on PR, let's keep it open then till there are still other open issues on the milestone. Maybe in the meantime some will set ubsan locally to confirm it.

[MichaelChirico]

Anyone want to have a go at making a UBSAN GitHub Codespace? Would be quite useful.

[ben-schwen]

Anyone want to have a go at making a UBSAN GitHub Codespace? Would be quite useful.

Can't we just mirror the current dev container and use the devel-clang docker image instead? Will do later!

[MichaelChirico]

As noted in #5850, there's still one issue left, I found it's from this bug:

Running test id 6.48
assign.c:912:47: runtime error: -1.84467e+19 is outside the range of representable values of type 'long'
assign.c:1040:21: runtime error: -1.84467e+19 is outside the range of representable values of type 'long'

data.table/inst/tests/nafill.Rraw

Line 185 in eace83a

test(6.48, identical(coerceFill(-(2^64)), list(NA_integer_, -(2^64), as.integer64(NA))), warning=c("precision lost","precision lost"))

[HughParsonage]

It appears to be the same problem, just with int64. I can attend to it after Christmas.

…

On Sat, 23 Dec 2023 at 2:56 am, Michael Chirico ***@***.***> wrote: As noted in #5850 <#5850>, there's still one issue left, I found it's from this bug: Running test id 6.48 assign.c:912:47: runtime error: -1.84467e+19 is outside the range of representable values of type 'long' assign.c:1040:21: runtime error: -1.84467e+19 is outside the range of representable values of type 'long' https://github.com/Rdatatable/data.table/blob/eace83a40c3ae3e95c52d6ec92ef193c2ded6e9b/inst/tests/nafill.Rraw#L185 — Reply to this email directly, view it on GitHub <#5832 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB54MDG3S7LGXNRHFGGOFO3YKWUUBAVCNFSM6AAAAABAYKG4USVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRXHA2DCNJWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[MichaelChirico]

It appears to be the same problem, just with int64. I can attend to it after Christmas.

NP, I have a fix just about ready.

[MichaelChirico]

Tried this as well to see if there's any residual issues, seems OK:

vals <- 2^(0:64)
vals <- c(-rev(vals), 0, vals)
vals <- c(vals, vals-1, vals+1)
data.table:::coerceAs(vals, bit64::as.integer64(1))

[MichaelChirico]

Think it's ready to go. Thanks! h/t @ben-schwen for getting the UBSAN codespace up & running to make debugging the last bit so much easier 🙇

[HughParsonage]

LGTM. I propose that this be now merged.

[MichaelChirico]

Thanks @HughParsonage! If you get a minute it might help to clean up the PR description to reflect the final state as submitted, I think I see a few notes about the interim state of the PR there.