Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uninitialized variable in parquetAcquireSampleRowsFunc #84

Closed
jk-intel opened this issue Jun 2, 2024 · 2 comments
Closed

Uninitialized variable in parquetAcquireSampleRowsFunc #84

jk-intel opened this issue Jun 2, 2024 · 2 comments

Comments

@jk-intel
Copy link

jk-intel commented Jun 2, 2024

Hi,
The variable fdw_private in the following code is uninitialized:

parquetAcquireSampleRowsFunc(Relation relation, int /* elevel */,
HeapTuple *rows, int targrows,
double *totalrows,
double *totaldeadrows)
{
...
ParquetFdwPlanState fdw_private;

Since ParquetFdwPlanState has trivial default constructor, the members of the struct have garbage values.
The following call to get_table_options() doesn't initialize all members either. E.g. the pointer attrs_sorted may not get initialized.

I suggest to memset() the fdw_private to zero immediately after the declaration:
ParquetFdwPlanState fdw_private;
memset(&fdw_private, 0, sizeof(ParquetFdwPlanState));

(In general, there are uninitialized-at-declaration pointers scattered throughout parquet_impl.cpp. Coding style is of course subjective :), but perhaps it's better to always initialize them.)

@za-arthur
Copy link
Contributor

za-arthur commented Jun 3, 2024

Thanks @jk-intel for another report. I agree it makes sense to zero out the struct before usage. I checked the code and it seems only parquetAcquireSampleRowsFunc() and parquetGetForeignRelSize() initialize that struct, and latter allocate it using palloc0, therefore there is no need to run memset additionally:

fdw_private = (ParquetFdwPlanState *) palloc0(sizeof(ParquetFdwPlanState));

I created a PR #85 with the fix.

@za-arthur
Copy link
Contributor

Closing since the PR was merged.

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

No branches or pull requests

2 participants