[clang] Enforce 1-based indexing for command line source locations (#139457)

Fixes #139375

Clang expects command line source locations to be provided using 1-based
indexing.
Currently, Clang does not reject zero as invalid argument for column or
line number, which can cause Clang to crash.

This commit extends validation in `ParsedSourceLocation::FromString` to
only accept (unsinged) non-zero integers.

Author: naveen-seth

clang/docs/ReleaseNotes.rst

@@ -859,6 +859,11 @@ clang-format
 - Add ``OneLineFormatOffRegex`` option for turning formatting off for one line.
 - Add ``SpaceAfterOperatorKeyword`` option.
 
+clang-refactor
+--------------
+- Reject `0` as column or line number in 1-based command-line source locations.
+  Fixes crash caused by `0` input in `-selection=<file>:<line>:<column>[-<line>:<column>]`. (#GH139457)
+
 libclang
 --------
 - Fixed a bug in ``clang_File_isEqual`` that sometimes led to different
@@ -877,6 +882,8 @@ libclang
 
 Code Completion
 ---------------
+- Reject `0` as column or line number in 1-based command-line source locations.
+  Fixes crash caused by `0` input in `-code-completion-at=<file>:<line>:<column>`. (#GH139457)
 
 Static Analyzer
 ---------------

clang/include/clang/Basic/DiagnosticDriverKinds.td

@@ -670,6 +670,10 @@ def note_drv_verify_prefix_spelling : Note<
   "-verify prefixes must start with a letter and contain only alphanumeric"
   " characters, hyphens, and underscores">;
 
+def note_command_line_code_loc_requirement
+    : Note<"-code-completion-at=<file>:<line>:<column> requires <line> and "
+           "<column> to be integers greater than zero">;
+
 def warn_drv_global_isel_incomplete : Warning<
   "-fglobal-isel support for the '%0' architecture is incomplete">,
   InGroup<GlobalISel>;

clang/include/clang/Frontend/CommandLineSourceLoc.h

@@ -24,7 +24,9 @@ namespace clang {
 /// A source location that has been parsed on the command line.
 struct ParsedSourceLocation {
   std::string FileName;
+  // The 1-based line number
   unsigned Line;
+  // The 1-based column number
   unsigned Column;
 
 public:
@@ -38,7 +40,8 @@ struct ParsedSourceLocation {
 
     // If both tail splits were valid integers, return success.
     if (!ColSplit.second.getAsInteger(10, PSL.Column) &&
-        !LineSplit.second.getAsInteger(10, PSL.Line)) {
+        !LineSplit.second.getAsInteger(10, PSL.Line) &&
+        !(PSL.Column == 0 || PSL.Line == 0)) {
       PSL.FileName = std::string(LineSplit.first);
 
       // On the command-line, stdin may be specified via "-". Inside the
@@ -89,8 +92,12 @@ struct ParsedSourceRange {
         // probably belongs to the filename which menas the whole
         // string should be parsed.
         RangeSplit.first = Str;
-      } else
+      } else {
+        // Column and line numbers are 1-based.
+        if (EndLine == 0 || EndColumn == 0)
+          return std::nullopt;
         HasEndLoc = true;
+      }
     }
     auto Begin = ParsedSourceLocation::FromString(RangeSplit.first);
     if (Begin.FileName.empty())

clang/lib/Frontend/CompilerInvocation.cpp

@@ -3112,9 +3112,11 @@ static bool ParseFrontendArgs(FrontendOptions &Opts, ArgList &Args,
   if (const Arg *A = Args.getLastArg(OPT_code_completion_at)) {
     Opts.CodeCompletionAt =
       ParsedSourceLocation::FromString(A->getValue());
-    if (Opts.CodeCompletionAt.FileName.empty())
+    if (Opts.CodeCompletionAt.FileName.empty()) {
       Diags.Report(diag::err_drv_invalid_value)
-        << A->getAsString(Args) << A->getValue();
+          << A->getAsString(Args) << A->getValue();
+      Diags.Report(diag::note_command_line_code_loc_requirement);
+    }
   }
 
   Opts.Plugins = Args.getAllArgValues(OPT_load);

clang/test/CodeCompletion/source-loc-zero.cpp

@@ -0,0 +1,11 @@
+// Regression test for #139375
+// Clang uses 1-based indexing for source locations given from the command-line.
+// Verify that Clang rejects 0 as an invalid value for line or column number.
+
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:0:1 %s -o - 2>&1 \
+// RUN:     | FileCheck -check-prefix=CHECK-DIAG %s
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:1:0 %s -o - 2>&1 \
+// RUN:     | FileCheck -check-prefix=CHECK-DIAG %s
+
+// CHECK-DIAG: error: invalid value '{{.*}}' in '-code-completion-at={{.*}}'
+// CHECK-NEXT: hint: -code-completion-at=<file>:<line>:<column> requires <line> and <column> to be integers greater than zero

clang/test/Refactor/source-loc-zero.cpp

@@ -0,0 +1,17 @@
+// Regression test for #139375
+// Clang uses 1-based indexing for source locations given from the command-line.
+// Verify that `clang-refactor` rejects 0 as an invalid value for line or column number.
+
+// For range start:
+// RUN: not clang-refactor local-rename -selection=%s:0:1-1:1 -new-name=test %s 2>&1 \
+// RUN:     | FileCheck -check-prefix=CHECK-DIAG %s
+// RUN: not clang-refactor local-rename -selection=%s:1:0-1:1 -new-name=test %s 2>&1 \
+// RUN:     | FileCheck -check-prefix=CHECK-DIAG %s
+
+// For range end:
+// RUN: not clang-refactor local-rename -selection=%s:1:1-0:1 -new-name=test %s 2>&1 \
+// RUN:     | FileCheck -check-prefix=CHECK-DIAG %s
+// RUN: not clang-refactor local-rename -selection=%s:1:1-1:0 -new-name=test %s 2>&1 \
+// RUN:     | FileCheck -check-prefix=CHECK-DIAG %s
+
+// CHECK-DIAG: error: '-selection' option must be specified using <file>:<line>:<column> or <file>:<line>:<column>-<line>:<column> format, where <line> and <column> are integers greater than zero.

clang/tools/clang-refactor/ClangRefactor.cpp

@@ -160,7 +160,8 @@ SourceSelectionArgument::fromString(StringRef Value) {
     return std::make_unique<SourceRangeSelectionArgument>(std::move(*Range));
   llvm::errs() << "error: '-selection' option must be specified using "
                   "<file>:<line>:<column> or "
-                  "<file>:<line>:<column>-<line>:<column> format\n";
+                  "<file>:<line>:<column>-<line>:<column> format, "
+                  "where <line> and <column> are integers greater than zero.\n";
   return nullptr;
 }