Fix undefined behavior in LDLT

[jwnimmer-tri]

When m==n the failed_rect.data() is nullptr, but then we still subtract some small integer from it. Doing arithmetic on a null pointer is undefined behavior. Clang's undefined Behavior sanitizer says ldlt_app.cxx:2420:38: runtime error: applying non-zero offset 18446744073709551536 to null pointer.

The copy_failed_rect ends up being a no-op because m==rfrom, but it's still UB to do arithmetic on nullptr, even if never de-referenced.

[jfowkes]

Thank you very much for the fix, @mjacobse would you be able to review this one?

[mjacobse]

Thanks, this should definitely be fixed and as far as I can see the proposed change would do it. I was just a bit bothered by needing a somewhat magic looking conditional just for this when it feels like there should be a more natural way to handle this special case. So I thought about it a bit and I believe this extra call for copying the blocks that contain both diagonal and rectangular parts can be written more succinctly and intuitively:

copy_failed_rect(
      get_nrow(nblk-1, m, block_size) - get_nrow(nblk-1, n, block_size),
      get_ncol(jblk, n, block_size), 0, cdata[jblk],
      failed_rect.data() + jfail*(m-n), m-n,
      &a[jblk*block_size*lda+n], lda
      );

This should be exactly equivalent since (nblk-1)*block_size + get_nrow(nblk-1, n, block_size) == n as I convinced myself with an illustration (see below, the hatched part is what the extra call is for). And it has the additional advantage that for m==n, the nullptr will only be incremented by 0 which should be fine, so it also fixes this issue. Let me know if I overlooked anything, otherwise I think I'd prefer this solution for better readability.

[jwnimmer-tri]

Yes, confirmed. I ran that new version through our UBSan test case that caught this, as well as our full Drake test suite, and everything passed. I've pushed that version now.

[mjacobse]

Great, thanks! Seems good to me but would be happy if you also take a look into the new version @jfowkes

[jfowkes]

Very neat, many thanks @mjacobse!

@nimgould you may want to incorporate this fix into galahad.

[nimgould]

Thank you, all, mods have been incorporated in galahad's multiprecision cpu version, and tests succeed