Skip to content

Fix segfault: Add mandatory 'location' validation in validator and reader#65

Merged
gmusya merged 2 commits into
lithium-tech:mainfrom
MutableFire:fix-missing-location
Jul 14, 2026
Merged

Fix segfault: Add mandatory 'location' validation in validator and reader#65
gmusya merged 2 commits into
lithium-tech:mainfrom
MutableFire:fix-missing-location

Conversation

@MutableFire

@MutableFire MutableFire commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Fix segmentation fault by implementing strict validation for the mandatory location option in both the FDW validator and the internal location reader. Instead of crashing when the option is missing, the invalid table creation is rejected now. When this table has already been created, throw an error on access.

@MutableFire MutableFire force-pushed the fix-missing-location branch 6 times, most recently from ae57f3c to 027c2ac Compare July 8, 2026 08:47
@MutableFire MutableFire force-pushed the fix-missing-location branch 3 times, most recently from 689cc08 to a5fb735 Compare July 8, 2026 12:05
@MutableFire MutableFire marked this pull request as ready for review July 8, 2026 13:41
@gmusya

gmusya commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

I think this change should be covered with test

@MutableFire MutableFire force-pushed the fix-missing-location branch 8 times, most recently from aefff95 to c5a2091 Compare July 13, 2026 15:09
@MutableFire

Copy link
Copy Markdown
Contributor Author

I think this change should be covered with test

Done.

Comment thread tea/smoke_test/negative_server_test.cpp Outdated
@MutableFire MutableFire force-pushed the fix-missing-location branch 3 times, most recently from 9122440 to c7805c3 Compare July 14, 2026 10:09
Comment thread tea/smoke_test/negative_server_test.cpp Outdated

PQclear(res);

const char* sql_empty_location =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to split this test into three separate tests

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread tea/smoke_test/negative_server_test.cpp Outdated
}

TEST_F(NegativeServer, MissingLocationOption) {
PGconn* raw_conn = conn_->Ptr();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should be skipped in Environment::GetTableType() == TestTableType::kExternal case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread tea/gpext/tea_fdw.c Outdated
if (list_length(options_list) != 1) {
if (list_length(options_list) == 0)
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("missing required option \"location\""),
errdetail("The TEA foreign data wrapper requires exactly one option: \"location\".")));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we declare some "The TEA foreign data wrapper requires exactly one option: \"location\"". variable and use it in both branches?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@MutableFire MutableFire force-pushed the fix-missing-location branch from c7805c3 to 7b5f650 Compare July 14, 2026 11:28
@MutableFire MutableFire force-pushed the fix-missing-location branch from 7b5f650 to 226d8b9 Compare July 14, 2026 11:30

@gmusya gmusya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gmusya gmusya merged commit 5ade5d9 into lithium-tech:main Jul 14, 2026
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants