This is an archive of the discontinued LLVM Phabricator instance.

[clang][CodeGen][UBSan] VLA size checking for unsigned integer parameter
ClosedPublic

Authored by AdamMagierFOSS on Dec 20 2021, 9:14 AM.

Details

Summary

The code generation for the UBSan VLA size check was qualified by a con-
dition that the parameter must be a signed integer, however the C spec
does not make any distinction that only signed integer parameters can be
used to declare a VLA, only qualifying that it must be greater than zero
if it is not a constant.

Diff Detail

Event Timeline

AdamMagierFOSS requested review of this revision.Dec 20 2021, 9:14 AM
AdamMagierFOSS created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2021, 9:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks, patch basically looks good. Minor requests and an observation for possible follow-up work.

clang/lib/CodeGen/CodeGenFunction.cpp
2247

Please continue to use lowercase names for local variables for consistency with the surrounding code. The rename to sizeExpr is good; you can then rename Size to size or sizeValue.

2259

Sema requires the bound expression to have integral type, so you don't need to do this.

2275

This would be a different bug, but should UBSan also be doing a bounds check if the type is larger than size_t?

Thank you for the feedback - I've added responses inline and I'll update the change to reflect the feedback.

clang/lib/CodeGen/CodeGenFunction.cpp
2247

Thank you for clarifying. I was unsure which style to stick to since I saw both "new style" code and "old style" code within the same scope, so I went with the "new style" to align with recent submissions to the clang project I looked at. It's no problem at all for me to switch my changes to the "old style" if that's preferred in this instance.

2259

I suspected this would be unnecessary - thank you for confirming, I'll remove this.

2275

Interesting point, I'd have to reread through the spec to give a precise/definitive answer. To keep this review focused I'll table the discussion for a separate forum.

Updating based on feedback from rjmccall.

rjmccall accepted this revision.Jan 11 2022, 2:01 PM

Okay, very minor requests, but otherwise LGTM; feel free to commit with these changes.

clang/lib/CodeGen/CodeGenFunction.cpp
2251

You can sink this into the if block.

2275

I'm pretty sure you should, but it's fine to do it in a different patch. Please leave a FIXME about it, though.

This revision is now accepted and ready to land.Jan 11 2022, 2:01 PM

Thanks once again for the feedback - I'll make the changes and commit directly.

clang/lib/CodeGen/CodeGenFunction.cpp
2251

Sure thing.

2275

My gut says the check should be performed as you say, but I can't prove it to myself in a satisfactory manner at the moment. I'll leave a FIXME for this as you requested.

alexfh added a subscriber: alexfh.Jan 21 2022, 8:54 AM

We've started seeing some tests dying with SIGILL (illegal instruction) after this patch. I'm working on a reduced repro, but please take a look, if you can find any issues yourself.

We've started seeing some tests dying with SIGILL (illegal instruction) after this patch. I'm working on a reduced repro, but please take a look, if you can find any issues yourself.

False alarm. All of these turned out to be real issues detected by the updated clang.