Fix uninitialized copy_data var (src/backend/commands/subscriptioncmds.c)

Started by Ranier Vilelaover 4 years ago6 messages

Ranier Vilela

ranier.vf@gmail.com

over 4 years ago

1 attachment(s)

Hi,

Not per Coverity!

About comments:
1. For drop, no "copy data"
2. Only refresh the added/*dropped* list of publications. (my emphasis)

The documentation says:
https://www.postgresql.org/docs/14/sql-altersubscription.html

"DROP PUBLICATION *publication_name*

Changes the list of subscribed publications. SET replaces the entire list
of publications with a new list, ADD adds additional publications, DROP
removes publications from the list of publications. See CREATE SUBSCRIPTION
<https://www.postgresql.org/docs/14/sql-createsubscription.html> for more
information. By default, this command will also act like REFRESH PUBLICATION,
except that in case of ADD or DROP, only the added or dropped publications
are refreshed.

*set_publication_option* specifies additional options for this operation.
The supported options are:
refresh (boolean)

When false, the command will not try to refresh table information. REFRESH
PUBLICATION should then be executed separately. The default is true.

Additionally, refresh options as described under REFRESH PUBLICATION may be
specified."
So, is allowed DROP PUBLICATION with (refresh = true)

I try some tests with subscription.sql:
CREATE SUBSCRIPTION regress_testsub3 CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false,
streaming = true);
+CREATE SUBSCRIPTION regress_testsub3 CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false,
streaming = true);
+WARNING:  tables were not subscribed, you will have to run ALTER
SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables

ALTER SUBSCRIPTION regress_testsub3 ENABLE;
ALTER SUBSCRIPTION regress_testsub3 REFRESH PUBLICATION;
+ALTER SUBSCRIPTION regress_testsub3 ENABLE;
+ALTER SUBSCRIPTION regress_testsub3 REFRESH PUBLICATION;
+ERROR:  could not connect to the publisher: connection to server at
"localhost" (::1), port 58080 failed: FATAL:  database
"regress_doesnotexist" does not exist

-- ok - delete active publication with refresh = true
ALTER SUBSCRIPTION regress_testsub3 DROP PUBLICATION testpub WITH (refresh
= true);
+-- ok - delete active publication with refresh = true
+ALTER SUBSCRIPTION regress_testsub3 DROP PUBLICATION testpub WITH (refresh
= true);
+ERROR:  subscription must contain at least one publication

I think this bug is live, for lack of tests with DROP PUBLICATION WITH
(refresh = true).

regards,
Ranier Vilela

Attachments:

fix_use_unitialized_copy_data_subscriptioncmds.patchapplication/octet-stream; name=fix_use_unitialized_copy_data_subscriptioncmds.patchDownload

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 75e195f286..ab31ef39dc 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -949,7 +949,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 		case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
 			{
 				bool		isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
-				bool		copy_data;
+				bool		copy_data = false;
 				bool		refresh;
 				List	   *publist;

Ranier Vilela

ranier.vf@gmail.com

over 4 years ago

In reply to: Ranier Vilela (#1)

Re: Fix uninitialized copy_data var (src/backend/commands/subscriptioncmds.c)

Em qua., 23 de jun. de 2021 às 14:38, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

Hi,

Not per Coverity!

About comments:
1. For drop, no "copy data"
2. Only refresh the added/*dropped* list of publications. (my emphasis)

The documentation says:
https://www.postgresql.org/docs/14/sql-altersubscription.html

"DROP PUBLICATION *publication_name*

Changes the list of subscribed publications. SET replaces the entire list
of publications with a new list, ADD adds additional publications, DROP
removes publications from the list of publications. See CREATE
SUBSCRIPTION
<https://www.postgresql.org/docs/14/sql-createsubscription.html> for more
information. By default, this command will also act like REFRESH
PUBLICATION, except that in case of ADD or DROP, only the added or
dropped publications are refreshed.

*set_publication_option* specifies additional options for this operation.
The supported options are:
refresh (boolean)

When false, the command will not try to refresh table information. REFRESH
PUBLICATION should then be executed separately. The default is true.

Additionally, refresh options as described under REFRESH PUBLICATION may
be specified."
So, is allowed DROP PUBLICATION with (refresh = true)
I try some tests with subscription.sql:
CREATE SUBSCRIPTION regress_testsub3 CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false,
streaming = true);
+CREATE SUBSCRIPTION regress_testsub3 CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false,
streaming = true);
+WARNING:  tables were not subscribed, you will have to run ALTER
SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables
ALTER SUBSCRIPTION regress_testsub3 ENABLE;
ALTER SUBSCRIPTION regress_testsub3 REFRESH PUBLICATION;
+ALTER SUBSCRIPTION regress_testsub3 ENABLE;
+ALTER SUBSCRIPTION regress_testsub3 REFRESH PUBLICATION;
+ERROR:  could not connect to the publisher: connection to server at
"localhost" (::1), port 58080 failed: FATAL:  database
"regress_doesnotexist" does not exist
-- ok - delete active publication with refresh = true
ALTER SUBSCRIPTION regress_testsub3 DROP PUBLICATION testpub WITH (refresh
= true);
+-- ok - delete active publication with refresh = true
+ALTER SUBSCRIPTION regress_testsub3 DROP PUBLICATION testpub WITH
(refresh = true);
+ERROR:  subscription must contain at least one publication
I think this bug is live, for lack of tests with DROP PUBLICATION WITH
(refresh = true).

https://github.com/postgres/postgres/commit/3af10943ce21450e299b3915b9cad47cd90369e9
fixes some issues with subscriptioncmds.c, but IMHO still lack this issue.

regards,
Ranier Vilela

Peter Smith

smithpb2250@gmail.com

over 4 years ago

In reply to: Ranier Vilela (#2)

Re: Fix uninitialized copy_data var (src/backend/commands/subscriptioncmds.c)

On Fri, Jun 25, 2021 at 11:55 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

https://github.com/postgres/postgres/commit/3af10943ce21450e299b3915b9cad47cd90369e9
fixes some issues with subscriptioncmds.c, but IMHO still lack this issue.

I have not tested this, and gcc gave no warnings about it, but just by
visual code inspection I do agree with you that this looks like a
problem, even in the latest code.

IIUC for the case ALTER_SUBSCRIPTION_DROP_PUBLICATION it looks like
the uninitialized copy_data local stack var would remain uninitialized
(undefined) still at the time it is passed at
AlterSubscription_refresh(sub, copy_data);

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

Michael Paquier

michael@paquier.xyz

over 4 years ago

In reply to: Peter Smith (#3)

Re: Fix uninitialized copy_data var (src/backend/commands/subscriptioncmds.c)

On Mon, Jun 28, 2021 at 10:17:55AM +1000, Peter Smith wrote:

IIUC for the case ALTER_SUBSCRIPTION_DROP_PUBLICATION it looks like
the uninitialized copy_data local stack var would remain uninitialized
(undefined) still at the time it is passed at
AlterSubscription_refresh(sub, copy_data);

Yes, that's wrong. AlterSubscription_refresh() would happily look at
this uninitialized value when performing a refresh with this command.
That's the only code path using parse_subscription_options() with this
pattern. Applied on HEAD.
--
Michael

Ranier Vilela

ranier.vf@gmail.com

over 4 years ago

In reply to: Peter Smith (#3)

Re: Fix uninitialized copy_data var (src/backend/commands/subscriptioncmds.c)

Em dom., 27 de jun. de 2021 às 21:18, Peter Smith <smithpb2250@gmail.com>
escreveu:

On Fri, Jun 25, 2021 at 11:55 PM Ranier Vilela <ranier.vf@gmail.com>
wrote:

https://github.com/postgres/postgres/commit/3af10943ce21450e299b3915b9cad47cd90369e9

fixes some issues with subscriptioncmds.c, but IMHO still lack this

issue.

I have not tested this, and gcc gave no warnings about it, but just by
visual code inspection I do agree with you that this looks like a
problem, even in the latest code.

IIUC for the case ALTER_SUBSCRIPTION_DROP_PUBLICATION it looks like
the uninitialized copy_data local stack var would remain uninitialized
(undefined) still at the time it is passed at
AlterSubscription_refresh(sub, copy_data);

Thanks Peter, for the review.

regards,
Ranier Vilela

Ranier Vilela

ranier.vf@gmail.com

over 4 years ago

In reply to: Michael Paquier (#4)

Re: Fix uninitialized copy_data var (src/backend/commands/subscriptioncmds.c)

Em seg., 28 de jun. de 2021 às 00:29, Michael Paquier <michael@paquier.xyz>
escreveu:

On Mon, Jun 28, 2021 at 10:17:55AM +1000, Peter Smith wrote:

IIUC for the case ALTER_SUBSCRIPTION_DROP_PUBLICATION it looks like
the uninitialized copy_data local stack var would remain uninitialized
(undefined) still at the time it is passed at
AlterSubscription_refresh(sub, copy_data);

Yes, that's wrong. AlterSubscription_refresh() would happily look at
this uninitialized value when performing a refresh with this command.
That's the only code path using parse_subscription_options() with this
pattern. Applied on HEAD.

Hi Michael,
Thank you for this comitt.

regards,
Ranier Vilela