search

adjust / parquet_fdw

Parquet foreign data wrapper for PostgreSQL
PostgreSQL License
351 stars 38 forks source link

Uninitialized variable in parquetAcquireSampleRowsFunc #84

Closed jk-intel closed 4 months ago

jk-intel commented 4 months ago

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 commented 4 months ago

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: https://github.com/adjust/parquet_fdw/blob/caf7471196b114d9b0b7c83e347014d8676dd816/src/parquet_impl.cpp#L1056

I created a PR https://github.com/adjust/parquet_fdw/pull/85 with the fix.

za-arthur commented 4 months ago

Closing since the PR was merged.