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.

Sign up for GitHub

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

Jump to bottom

Fix coverity scan logically dead code #6451

Merged
merged 1 commit into from
Dec 22, 2023
Merged

Fix coverity scan logically dead code #6451

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 5 additions & 19 deletions tsl/src/continuous_aggs/repair.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ cagg_rebuild_view_definition(ContinuousAgg *agg, Hypertable *mat_ht, bool force_
ListCell *lc1, *lc2;
int sec_ctx;
Oid uid, saved_uid;

bool finalized = ContinuousAggIsFinalized(agg);

if (!finalized)
{
ereport(WARNING,
Expand Down Expand Up @@ -131,24 +131,10 @@ cagg_rebuild_view_definition(ContinuousAgg *agg, Hypertable *mat_ht, bool force_
fqi.finalized = finalized;
finalizequery_init(&fqi, direct_query, &mattblinfo);

/*
* Add any internal columns needed for materialization based
* on the user query's table.
*/
if (!finalized)
mattablecolumninfo_addinternal(&mattblinfo);

Query *view_query = NULL;
if (rebuild_cagg_with_joins)
{
view_query = finalizequery_get_select_query(&fqi,
mattblinfo.matcollist,
&mataddress,
NameStr(mat_ht->fd.table_name));
}
else
view_query =
finalizequery_get_select_query(&fqi, mattblinfo.matcollist, &mataddress, relname);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering what happens in this case: Since we now remove the else statement, view_query could be uninitialized in line 149. If !agg->data.materialized_only in line 153 is also false, the variable stays uninitialized and we will access the NULL pointer in line 181.

Even if the condition is true, passing the uninitialized view_query to build_union_query looks suspicious. However, our test cases do not fail and these branches are marked as dead code. But could you double-check that this is not a problem before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not happen because in the current logic the only way to this happen is if the finalized=false and in that case we are exiting earlier of the function emitting a warning message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway removed the branch because it become useless at this point.

Query *view_query = view_query = finalizequery_get_select_query(&fqi,
mattblinfo.matcollist,
&mataddress,
NameStr(mat_ht->fd.table_name));

if (!agg->data.materialized_only)
{
Expand Down