[AST] Fix UB appearing due to strict aliasing rule violation #85286
Conversation
This fixes the following tests which are now crashing due to hitting unreachable "no buffer containing location found" in `SourceManager::findBufferContainingLoc`: - attr/attr_dynamic_member_lookup.swift - AutoDiff/Sema/DerivativeRegistrationCrossModule/main.swift - Constraints/construction.swift - Constraints/members.swift - NameLookup/accessibility.swift - Sema/call_as_function_simple.swift The root cause of the crash is as follows. All the tests involve emitting `InaccessibleMemberFailure` error diagnostic (see `InaccessibleMemberFailure::diagnoseAsError`). When `DeclNameLoc nameLoc` is initialized via default constructor and is not re-assigned with smth meaningful later, the check `nameLoc.isValid()` returns `true` even though it should be `false`. It turns out that we treat `const void *LocationInfo` as `SourceLoc` and hope that if `LocationInfo` is `nullptr`, casting this to `SourceLoc` would produce a "default" `SourceLoc` with `Pointer` member set to `nullptr`. But such a cast (see `getSourceLocs()` member of `DeclNameLoc`) is undefined behavior. So, the compiler assumes that `Pointer` member of `SourceLoc` we try to cast to is not `nullptr`, which leads to `nameLoc.isValid()` being mistakenly computed as `true`. This patch resolves the issue by handling this special case separately.
|
Tagging @JaapWijnen |
|
preset=buildbot_incremental_asan_ubsan,tools=RDA,stdlib=RDA |
|
@swift-ci Please test |
|
@slavapestov BTW, could you please elaborate if we expect existing main CI jobs enabling UBSan to catch such issues? I see that we have "[main] 7. OSS - LLDB - ASAN + UBSAN - Release - macOS" build job https://ci.swift.org/job/oss-lldb-asan-macos/ (the URL does not include "ubsan" word though). I suppose that this is intended to catch such kinds of issues, isn't it? Note: to be honest, I was unable to get a message from UBSan locally as well, but just because I was unable to properly set up sanitizer-enabled build in my local development Linux environment in a reasonable timeframe :) I'm sure though that this must be catched by UBSan and we should see the failure on sanitizer-enabled CI jobs. It would be super helpful if we could have a nice way to catch such bugs on CI since trying to debug them locally is painful and time-consuming. |
|
I'm not familiar with UBSan and ASan, sorry. @shahmishal might know. |
DougGregor
left a comment
DougGregor
left a comment
There was a problem hiding this comment.
Wow. Excellent debugging on this.
|
@swift-ci Please test Windows platform |
@shahmishal Could you please elaborate if we expect existing main CI jobs enabling UBSan to catch UB in the compiler? I see that we have "[main] 7. OSS - LLDB - ASAN + UBSAN - Release - macOS" build job https://ci.swift.org/job/oss-lldb-asan-macos/ (the URL does not include "ubsan" word though). I suppose that this is intended to catch such kinds of issues, isn't it? It would be super helpful if we could have a nice way to catch such bugs on CI since trying to debug them locally is painful and time-consuming. |
…ng#85286) This fixes the following tests which are now crashing due to hitting unreachable "no buffer containing location found" in `SourceManager::findBufferContainingLoc`: - attr/attr_dynamic_member_lookup.swift - AutoDiff/Sema/DerivativeRegistrationCrossModule/main.swift - Constraints/construction.swift - Constraints/members.swift - NameLookup/accessibility.swift - Sema/call_as_function_simple.swift The root cause of the crash is as follows. All the tests involve emitting `InaccessibleMemberFailure` error diagnostic (see `InaccessibleMemberFailure::diagnoseAsError`). When `DeclNameLoc nameLoc` is initialized via default constructor and is not re-assigned with smth meaningful later, the check `nameLoc.isValid()` returns `true` even though it should be `false`. It turns out that we treat `const void *LocationInfo` as `SourceLoc` and hope that if `LocationInfo` is `nullptr`, casting this to `SourceLoc` would produce a "default" `SourceLoc` with `Pointer` member set to `nullptr`. But such a cast (see `getSourceLocs()` member of `DeclNameLoc`) is undefined behavior. So, the compiler assumes that `Pointer` member of `SourceLoc` we try to cast to is not `nullptr` (due to strict aliasing rule), which leads to `nameLoc.isValid()` being mistakenly computed as `true`. This patch resolves the issue by handling this special case separately.
3 participants
This fixes the following tests which are now crashing due to hitting unreachable "no buffer containing location found" in
SourceManager::findBufferContainingLoc:The root cause of the crash is as follows. All the tests involve emitting
InaccessibleMemberFailureerror diagnostic (seeInaccessibleMemberFailure::diagnoseAsError). WhenDeclNameLoc nameLocis initialized via default constructor and is not re-assigned with smth meaningful later, the checknameLoc.isValid()returnstrueeven though it should befalse. It turns out that we treatconst void *LocationInfoasSourceLocand hope that ifLocationInfoisnullptr, casting this toSourceLocwould produce a "default"SourceLocwithPointermember set tonullptr. But such a cast (seegetSourceLocs()member ofDeclNameLoc) is undefined behavior. So, the compiler assumes thatPointermember ofSourceLocwe try to cast to is notnullptr(due to strict aliasing rule), which leads tonameLoc.isValid()being mistakenly computed astrue.This patch resolves the issue by handling this special case separately.