Skip to content

Commit 453073f

Browse files
ziqingluo-90ziqingluo-90
committed
Address comments
- Make the diagnostic message simply saying "unsafe [conversion operation] of '__null_terminated' type"; further explanation is put in notes. - Use `warn_unsafe_buffer_operation` to consistently test if CXX Safe Buffer is enabled. Add an helper function for the test. - Remove the warning on using C++ named cast to obtain __null_terminated pointers. - Added tests for good cases of C-Style casts. - Consistently use isBoundsSafetyAttributeOnlyMode for code involving bounds attributes but not shared with full Bounds Safety mode.
1 parent e8e5a60 commit 453073f

File tree

6 files changed

+90
-76
lines changed

6 files changed

+90
-76
lines changed

‎clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13610,12 +13610,11 @@ def warn_unsafe_assign_to_null_terminated_pointer : Warning<
1361013610
"conversion to|"
1361113611
"initialization of a pointer of|"
1361213612
"assignment to a pointer of|"
13613-
"casting to}2 '__null_terminated' type; "
13614-
"only '__null_terminated' pointers, string literals, and 'std::string::c_str' calls are compatible with '__null_terminated' pointers">,
13615-
InGroup<UnsafeBufferUsage>, DefaultIgnore;
13616-
def warn_unsafe_named_cast_to_null_terminated_pointer : Warning<
13617-
"C++ named cast to '__null_terminated' type is unsafe">,
13613+
"casting to}2 '__null_terminated' type">,
1361813614
InGroup<UnsafeBufferUsage>, DefaultIgnore;
13615+
def note_unsafe_assign_to_null_terminated_pointer : Note<
13616+
"the source pointer may not point to null-terminated data; "
13617+
"only '__null_terminated' pointers, string literals, and 'std::string::c_str' calls are compatible with '__null_terminated' pointers">;
1361913618
#ifndef NDEBUG
1362013619
// Not a user-facing diagnostic. Useful for debugging false negatives in
1362113620
// -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).

‎clang/include/clang/Sema/Sema.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1560,6 +1560,11 @@ class Sema final : public SemaBase {
15601560
/// be controlled with \#pragma clang abi_ptr_attr set(*ATTR*). Defaults to
15611561
/// __single in user code and __unsafe_indexable in system headers.
15621562
BoundsSafetyPointerAttributes CurPointerAbi;
1563+
1564+
/// \returns true iff `-Wunsafe-buffer-usage` is enabled for `Loc`.
1565+
bool isCXXSafeBuffersEnabledAt(SourceLocation Loc) {
1566+
return !Diags.isIgnored(diag::warn_unsafe_buffer_operation, Loc);
1567+
}
15631568
/* TO_UPSTREAM(BoundsSafety) OFF */
15641569

15651570
/// pragma clang section kind

‎clang/lib/Sema/SemaCast.cpp

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -240,13 +240,11 @@ namespace {
240240
DstPointerType->getTerminatorValue(Self.Context).isZero();
241241
if (Self.LangOpts.isBoundsSafetyAttributeOnlyMode() &&
242242
Self.LangOpts.CPlusPlus &&
243-
!Self.Diags.isIgnored(
244-
diag::warn_unsafe_assign_to_null_terminated_pointer,
245-
SrcExpr.get()->getExprLoc())) {
243+
Self.isCXXSafeBuffersEnabledAt(OpRange.getBegin())) {
246244
// In C++ Safe Buffers/Bounds Safety interop mode, the compiler
247245
// reports a warning under -Wunsafe-buffer-usage:
248246
DiagKind = diag::warn_unsafe_assign_to_null_terminated_pointer;
249-
isInvalid = true;
247+
isInvalid = false;
250248
break;
251249
}
252250
if (Self.getLangOpts().BoundsSafetyRelaxedSystemHeaders) {
@@ -300,6 +298,9 @@ namespace {
300298
Self.Diag(OpRange.getBegin(), DiagKind)
301299
<< SrcType << DestType << AssignmentAction::Casting;
302300
}
301+
if (DiagKind == diag::warn_unsafe_assign_to_null_terminated_pointer)
302+
Self.Diag(OpRange.getBegin(),
303+
diag::note_unsafe_assign_to_null_terminated_pointer);
303304

304305
Self.TryFixAssigningNullTerminatedToBidiIndexableExpr(SrcExpr.get(),
305306
DestType);
@@ -587,17 +588,6 @@ Sema::BuildCXXNamedCast(SourceLocation OpLoc, tok::TokenKind Kind,
587588

588589
Op.checkQualifiedDestType();
589590

590-
// In C++ Safe Buffers/Bounds Safety interop mode, forbidding for now all
591-
// named cast to __null_terminated pointer type.
592-
if (DestType->isPointerType() && LangOpts.hasBoundsSafetyAttributes() &&
593-
!Diags.isIgnored(diag::warn_unsafe_named_cast_to_null_terminated_pointer,
594-
E->getBeginLoc())) {
595-
if (DestType->getAs<ValueTerminatedType>()) {
596-
Diag(E->getBeginLoc(),
597-
diag::warn_unsafe_named_cast_to_null_terminated_pointer);
598-
return ExprError();
599-
}
600-
}
601591
switch (Kind) {
602592
default: llvm_unreachable("Unknown C++ cast!");
603593

‎clang/lib/Sema/SemaExpr.cpp

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12195,24 +12195,20 @@ Sema::CheckValueTerminatedAssignmentConstraints(QualType LHSType,
1219512195
}
1219612196
// If in C++ Safe Buffers/Bounds Safety interoperation mode, the RHS can
1219712197
// be 'std::string::c_str':
12198-
bool SafeBuffersEnabled = !Diags.isIgnored(
12199-
diag::warn_unsafe_assign_to_null_terminated_pointer,
12200-
RHSExpr->getBeginLoc());
12201-
12202-
if (LangOpts.CPlusPlus && SafeBuffersEnabled) {
12198+
if (LangOpts.CPlusPlus &&
12199+
isCXXSafeBuffersEnabledAt(RHSExpr->getBeginLoc())) {
1220312200
if (const auto *MCE =
1220412201
dyn_cast<CXXMemberCallExpr>(RHSExpr->IgnoreParenImpCasts())) {
1220512202
IdentifierInfo *MethodDeclII = nullptr, *ObjTypeII = nullptr;
1220612203

1220712204
if (MCE->getMethodDecl())
1220812205
MethodDeclII = MCE->getMethodDecl()->getIdentifier();
1220912206
if (const auto *RecordDecl =
12210-
MCE->getObjectType()->getAsRecordDecl()) {
12207+
MCE->getObjectType()->getAsRecordDecl();
12208+
RecordDecl && RecordDecl->isInStdNamespace())
1221112209
// If not in std namespace, let `ObjTypeII` be null so that the
1221212210
// match fails:
12213-
if (RecordDecl->isInStdNamespace())
12214-
ObjTypeII = RecordDecl->getIdentifier();
12215-
}
12211+
ObjTypeII = RecordDecl->getIdentifier();
1221612212
if (MethodDeclII && ObjTypeII &&
1221712213
MethodDeclII->getName() == "c_str" &&
1221812214
ObjTypeII->getName() == "basic_string")
@@ -20810,9 +20806,7 @@ bool Sema::DiagnoseAssignmentResult(AssignConvertType ConvTy,
2081020806
IsDstNullTerm =
2081120807
DstPointerType->getTerminatorValue(getASTContext()).isZero();
2081220808
if (getLangOpts().isBoundsSafetyAttributeOnlyMode() &&
20813-
getLangOpts().CPlusPlus &&
20814-
!Diags.isIgnored(diag::warn_unsafe_assign_to_null_terminated_pointer,
20815-
Loc)) {
20809+
getLangOpts().CPlusPlus && isCXXSafeBuffersEnabledAt(Loc)) {
2081620810
// In C++ Safe Buffers/Bounds Safety interop mode, the compiler reports a
2081720811
// warning under -Wunsafe-buffer-usage:
2081820812
DiagKind = diag::warn_unsafe_assign_to_null_terminated_pointer;
@@ -21254,6 +21248,11 @@ bool Sema::DiagnoseAssignmentResult(AssignConvertType ConvTy,
2125421248
}
2125521249
}
2125621250

21251+
/* TO_UPSTREAM(BoundsSafety) ON*/
21252+
if (DiagKind == diag::warn_unsafe_assign_to_null_terminated_pointer)
21253+
Diag(Loc, diag::note_unsafe_assign_to_null_terminated_pointer);
21254+
/* TO_UPSTREAM(BoundsSafety) OFF*/
21255+
2125721256
if (IsSrcNullTerm) {
2125821257
TryFixAssigningNullTerminatedToImplicitBidiIndexablePtr(Assignee, SrcExpr,
2125921258
DstType, Action);

‎clang/lib/Sema/SemaExprCXX.cpp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4225,16 +4225,14 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType,
42254225
const ImplicitConversionSequence &ICS,
42264226
AssignmentAction Action,
42274227
CheckedConversionKind CCK) {
4228-
if (ToType->isPointerType() && LangOpts.hasBoundsSafetyAttributes() &&
4229-
!Diags.isIgnored(diag::warn_unsafe_assign_to_null_terminated_pointer,
4230-
From->getBeginLoc())) {
4228+
if (ToType->isPointerType() && LangOpts.isBoundsSafetyAttributeOnlyMode() &&
4229+
isCXXSafeBuffersEnabledAt(From->getBeginLoc())) {
42314230
// In C++ Safe Buffers/Bounds Safety interop mode, warn about implicit
42324231
// conversions from non-`__null_terminated` to `__null_terminated`:
42334232
AssignConvertType ConvTy =
42344233
CheckValueTerminatedAssignmentConstraints(ToType, From);
4235-
if (DiagnoseAssignmentResult(ConvTy, From->getExprLoc(), ToType,
4236-
From->getType(), From, Action))
4237-
return ExprError();
4234+
DiagnoseAssignmentResult(ConvTy, From->getExprLoc(), ToType,
4235+
From->getType(), From, Action);
42384236
}
42394237
// C++ [over.match.oper]p7: [...] operands of class type are converted [...]
42404238
if (CCK == CheckedConversionKind::ForBuiltinOverloadedOp &&

0 commit comments

Comments
 (0)