Skip to content

Commit 034c2db

Browse files
jnthntatumcopybara-github
authored andcommitted
TypeChecker updates to filter bad line information.
Add additional checks in line offset computation to avoid integer overflows for ASTs with bad postion maps. PiperOrigin-RevId: 697761888
1 parent 9687039 commit 034c2db

File tree

2 files changed

+74
-6
lines changed

2 files changed

+74
-6
lines changed

checker/internal/type_checker_impl.cc

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,21 +75,31 @@ SourceLocation ComputeSourceLocation(const AstImpl& ast, int64_t expr_id) {
7575
return SourceLocation{};
7676
}
7777
int32_t absolute_position = iter->second;
78+
if (absolute_position < 0) {
79+
return SourceLocation{};
80+
}
81+
82+
// Find the first line offset that is greater than the absolute position.
7883
int32_t line_idx = -1;
84+
int32_t offset = 0;
7985
for (int32_t i = 0; i < source_info.line_offsets().size(); ++i) {
80-
int32_t offset = source_info.line_offsets()[i];
81-
if (absolute_position < offset) {
86+
int32_t next_offset = source_info.line_offsets()[i];
87+
if (next_offset <= offset) {
88+
// Line offset is not monotonically increasing, so line information is
89+
// invalid.
90+
return SourceLocation{};
91+
}
92+
if (absolute_position < next_offset) {
8293
line_idx = i;
8394
break;
8495
}
96+
offset = next_offset;
8597
}
8698

87-
if (line_idx <= 0 || line_idx >= source_info.line_offsets().size()) {
88-
return SourceLocation{1, absolute_position};
99+
if (line_idx < 0 || line_idx >= source_info.line_offsets().size()) {
100+
return SourceLocation{};
89101
}
90102

91-
auto offset = source_info.line_offsets()[line_idx - 1];
92-
93103
int32_t rel_position = absolute_position - offset;
94104

95105
return SourceLocation{line_idx + 1, rel_position};

checker/internal/type_checker_impl_test.cc

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ using ::testing::Eq;
6868
using ::testing::IsEmpty;
6969
using ::testing::Pair;
7070
using ::testing::Property;
71+
using ::testing::SizeIs;
7172

7273
using AstType = ast_internal::Type;
7374
using Severity = TypeCheckIssue::Severity;
@@ -1281,6 +1282,63 @@ TEST(TypeCheckerImplTest, ExpectedTypeDoesntMatch) {
12811282
"expected type 'map<string, string>' but found 'map<string, int>'")));
12821283
}
12831284

1285+
TEST(TypeCheckerImplTest, BadSourcePosition) {
1286+
google::protobuf::Arena arena;
1287+
TypeCheckEnv env(GetSharedTestingDescriptorPool());
1288+
1289+
TypeCheckerImpl impl(std::move(env));
1290+
ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("foo"));
1291+
auto& ast_impl = AstImpl::CastFromPublicAst(*ast);
1292+
ast_impl.source_info().mutable_positions()[1] = -42;
1293+
ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast)));
1294+
ASSERT_OK_AND_ASSIGN(auto source, NewSource("foo"));
1295+
1296+
EXPECT_FALSE(result.IsValid());
1297+
ASSERT_THAT(result.GetIssues(), SizeIs(1));
1298+
1299+
EXPECT_EQ(
1300+
result.GetIssues()[0].ToDisplayString(*source),
1301+
"ERROR: <input>:-1:-1: undeclared reference to 'foo' (in container '')");
1302+
}
1303+
1304+
TEST(TypeCheckerImplTest, BadLineOffsets) {
1305+
google::protobuf::Arena arena;
1306+
TypeCheckEnv env(GetSharedTestingDescriptorPool());
1307+
1308+
TypeCheckerImpl impl(std::move(env));
1309+
ASSERT_OK_AND_ASSIGN(auto source, NewSource("\nfoo"));
1310+
1311+
{
1312+
ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("\nfoo"));
1313+
auto& ast_impl = AstImpl::CastFromPublicAst(*ast);
1314+
ast_impl.source_info().mutable_line_offsets()[1] = 1;
1315+
ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast)));
1316+
1317+
EXPECT_FALSE(result.IsValid());
1318+
ASSERT_THAT(result.GetIssues(), SizeIs(1));
1319+
1320+
EXPECT_EQ(result.GetIssues()[0].ToDisplayString(*source),
1321+
"ERROR: <input>:-1:-1: undeclared reference to 'foo' (in "
1322+
"container '')");
1323+
}
1324+
{
1325+
ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("\nfoo"));
1326+
auto& ast_impl = AstImpl::CastFromPublicAst(*ast);
1327+
ast_impl.source_info().mutable_line_offsets().clear();
1328+
ast_impl.source_info().mutable_line_offsets().push_back(-1);
1329+
ast_impl.source_info().mutable_line_offsets().push_back(2);
1330+
1331+
ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast)));
1332+
1333+
EXPECT_FALSE(result.IsValid());
1334+
ASSERT_THAT(result.GetIssues(), SizeIs(1));
1335+
1336+
EXPECT_EQ(result.GetIssues()[0].ToDisplayString(*source),
1337+
"ERROR: <input>:-1:-1: undeclared reference to 'foo' (in "
1338+
"container '')");
1339+
}
1340+
}
1341+
12841342
TEST(TypeCheckerImplTest, ContainerLookupForMessageCreation) {
12851343
TypeCheckEnv env(GetSharedTestingDescriptorPool());
12861344
env.set_container("google.protobuf");

0 commit comments

Comments
 (0)