Thread: Re: [BUGS] Bug#333854: pg_group file update problems
I have developed a small patch to 8.0.X that fixes your reported
problem:
test=> CREATE GROUP g1;
CREATE GROUP
test=> CREATE USER u1 IN GROUP g1;
CREATE USER
test=> \! cat /u/pg/data/global/pg_group
"g1" "u1"
test=> CREATE USER u2 IN GROUP g1;
CREATE USER
test=> \! cat /u/pg/data/global/pg_group
"g1" "u1" "u2"
test=> ALTER USER u2 RENAME TO u3;
ALTER USER
test=> \! cat /u/pg/data/global/pg_group
"g1" "u1" "u3"
In the patch, notice the old comment that suggests we might need to use
CommandCounterIncrement().
This sesms safe to apply to back branches.
---------------------------------------------------------------------------
Dennis Vshivkov wrote:
> Package: postgresql-8.0
> Version: 8.0.3-13
> Severity: important
> Tags: patch, upstream
>
> Here's the problem:
>
> db=# CREATE GROUP g1;
> CREATE GROUP
> db=# CREATE USER u1 IN GROUP g1; (1)
> CREATE USER
>
> # cat /var/lib/postgresql/8.0/main/global/pg_group
> #
>
> The file gets rewritten, but the group `g1' line does not get
> added to the file. Continue:
>
> db=# CREATE USER u2 IN GROUP g1; (2)
> CREATE USER
>
> # cat /var/lib/postgresql/8.0/main/global/pg_group
> "g1" "u1"
> #
>
> Now the line is there, but it lacks the latest member. Consider
> this also:
>
> db=# ALTER USER u2 RENAME TO u3; (3)
> ALTER USER
>
> # cat /var/lib/postgresql/8.0/main/global/pg_group
> "g1" "u1" "u2"
> #
>
> The problem is that the code that updates pg_group file resolves
> group membership through the system user catalogue cache. The
> file update happens shortly before the commit, but the caches
> only see updates after the commit. Because of this, new users
> or changes in users' names often do not make it to pg_group.
> That leads to mysterious authentication failures subsequently.
> The problem can also have security implications for certain
> pg_hba.conf arrangements.
>
> The attached `98-6-pg_group-stale-data-fix.patch' makes the code
> in question access the system user table directly and thus fixes
> the cases (1) and (2), however (3) is doubly ill: the user
> renaming code does not even trigger a pg_group file update.
> Hence the other patch, `98-5-rename-user-update-pg_group.patch'.
>
> A byproduct of the main fix is removal of an unlikely system
> cache reference leak which happens if a group member name
> contains a newline.
>
> The problems were found and the fixes were done for PostgreSQL
> 8.0.3 release. The flaws seem intact in 8.0.4 source code, too.
>
> Hope this helps.
>
> --
> /Awesome Walrus <walrus@amur.ru>
[ Attachment, skipping... ]
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Index: src/backend/commands/user.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/user.c,v
retrieving revision 1.147
diff -c -c -r1.147 user.c
*** src/backend/commands/user.c 31 Dec 2004 21:59:42 -0000 1.147
--- src/backend/commands/user.c 14 Oct 2005 15:42:17 -0000
***************
*** 175,183 ****
/*
* Read pg_group and write the file. Note we use SnapshotSelf to
! * ensure we see all effects of current transaction. (Perhaps could
! * do a CommandCounterIncrement beforehand, instead?)
*/
scan = heap_beginscan(grel, SnapshotSelf, 0, NULL);
while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
--- 175,183 ----
/*
* Read pg_group and write the file. Note we use SnapshotSelf to
! * ensure we see all effects of current transaction.
*/
+ CommandCounterIncrement();
scan = heap_beginscan(grel, SnapshotSelf, 0, NULL);
while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
***************
*** 781,786 ****
--- 781,787 ----
* Set flag to update flat password file at commit.
*/
user_file_update_needed();
+ group_file_update_needed();
}
***************
*** 1200,1205 ****
--- 1201,1207 ----
* Set flag to update flat password file at commit.
*/
user_file_update_needed();
+ group_file_update_needed();
}
***************
*** 1286,1291 ****
--- 1288,1294 ----
heap_close(rel, NoLock);
user_file_update_needed();
+ group_file_update_needed();
}
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> In the patch, notice the old comment that suggests we might need to use
> CommandCounterIncrement().
... which you failed to fix in any meaningful way. I'd suggest
/*
* Advance the commmand counter to ensure we see all results
* of current transaction.
*/
CommandCounterIncrement();
and then change SnapshotSelf to SnapshotNow, since there's no longer any
reason for it to be special. Compare to CVS tip which already does it
that way. See also the identical code in write_user_file (which perhaps
has no bug, but ISTM it should stay identical).
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > In the patch, notice the old comment that suggests we might need to use > > CommandCounterIncrement(). > > ... which you failed to fix in any meaningful way. I'd suggest > > /* > * Advance the commmand counter to ensure we see all results > * of current transaction. > */ > CommandCounterIncrement(); > > and then change SnapshotSelf to SnapshotNow, since there's no longer any > reason for it to be special. Compare to CVS tip which already does it > that way. See also the identical code in write_user_file (which perhaps > has no bug, but ISTM it should stay identical). OK, updated patch. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/backend/commands/user.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/commands/user.c,v retrieving revision 1.147 diff -c -c -r1.147 user.c *** src/backend/commands/user.c 31 Dec 2004 21:59:42 -0000 1.147 --- src/backend/commands/user.c 14 Oct 2005 17:36:54 -0000 *************** *** 175,184 **** /* * Read pg_group and write the file. Note we use SnapshotSelf to ! * ensure we see all effects of current transaction. (Perhaps could ! * do a CommandCounterIncrement beforehand, instead?) */ ! scan = heap_beginscan(grel, SnapshotSelf, 0, NULL); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Datum datum, --- 175,184 ---- /* * Read pg_group and write the file. Note we use SnapshotSelf to ! * ensure we see all effects of current transaction. */ ! CommandCounterIncrement(); /* see our current changes */ ! scan = heap_beginscan(grel, SnapshotNow, 0, NULL); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Datum datum, *************** *** 322,331 **** /* * Read pg_shadow and write the file. Note we use SnapshotSelf to ! * ensure we see all effects of current transaction. (Perhaps could ! * do a CommandCounterIncrement beforehand, instead?) */ ! scan = heap_beginscan(urel, SnapshotSelf, 0, NULL); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Datum datum; --- 322,331 ---- /* * Read pg_shadow and write the file. Note we use SnapshotSelf to ! * ensure we see all effects of current transaction. */ ! CommandCounterIncrement(); /* see our current changes */ ! scan = heap_beginscan(urel, SnapshotNow, 0, NULL); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Datum datum; *************** *** 781,786 **** --- 781,787 ---- * Set flag to update flat password file at commit. */ user_file_update_needed(); + group_file_update_needed(); } *************** *** 1200,1205 **** --- 1201,1207 ---- * Set flag to update flat password file at commit. */ user_file_update_needed(); + group_file_update_needed(); } *************** *** 1286,1291 **** --- 1288,1294 ---- heap_close(rel, NoLock); user_file_update_needed(); + group_file_update_needed(); }
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> OK, updated patch.
I was sort of hoping that you would make the comments agree with the
code...
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > OK, updated patch. > > I was sort of hoping that you would make the comments agree with the > code... Oh, you really read those comments? Fixed and attached. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/backend/commands/user.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/commands/user.c,v retrieving revision 1.147 diff -c -c -r1.147 user.c *** src/backend/commands/user.c 31 Dec 2004 21:59:42 -0000 1.147 --- src/backend/commands/user.c 14 Oct 2005 21:18:42 -0000 *************** *** 174,184 **** errmsg("could not write to temporary file \"%s\": %m", tempname))); /* ! * Read pg_group and write the file. Note we use SnapshotSelf to ! * ensure we see all effects of current transaction. (Perhaps could ! * do a CommandCounterIncrement beforehand, instead?) */ ! scan = heap_beginscan(grel, SnapshotSelf, 0, NULL); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Datum datum, --- 174,183 ---- errmsg("could not write to temporary file \"%s\": %m", tempname))); /* ! * Read pg_group and write the file */ ! CommandCounterIncrement(); /* see our current changes */ ! scan = heap_beginscan(grel, SnapshotNow, 0, NULL); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Datum datum, *************** *** 321,331 **** errmsg("could not write to temporary file \"%s\": %m", tempname))); /* ! * Read pg_shadow and write the file. Note we use SnapshotSelf to ! * ensure we see all effects of current transaction. (Perhaps could ! * do a CommandCounterIncrement beforehand, instead?) */ ! scan = heap_beginscan(urel, SnapshotSelf, 0, NULL); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Datum datum; --- 320,329 ---- errmsg("could not write to temporary file \"%s\": %m", tempname))); /* ! * Read pg_shadow and write the file */ ! CommandCounterIncrement(); /* see our current changes */ ! scan = heap_beginscan(urel, SnapshotNow, 0, NULL); while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Datum datum; *************** *** 781,786 **** --- 779,785 ---- * Set flag to update flat password file at commit. */ user_file_update_needed(); + group_file_update_needed(); } *************** *** 1200,1205 **** --- 1199,1205 ---- * Set flag to update flat password file at commit. */ user_file_update_needed(); + group_file_update_needed(); } *************** *** 1286,1291 **** --- 1286,1292 ---- heap_close(rel, NoLock); user_file_update_needed(); + group_file_update_needed(); }
This bug will be fixed in the next 8.0.X release. I have not fixed 7.4.X because the risk/benefit does not warrant it, and 8.1 does not have this problem. --------------------------------------------------------------------------- Bruce Momjian wrote: > > I have developed a small patch to 8.0.X that fixes your reported > problem: > > test=> CREATE GROUP g1; > CREATE GROUP > > test=> CREATE USER u1 IN GROUP g1; > CREATE USER > test=> \! cat /u/pg/data/global/pg_group > "g1" "u1" > > test=> CREATE USER u2 IN GROUP g1; > CREATE USER > test=> \! cat /u/pg/data/global/pg_group > "g1" "u1" "u2" > > test=> ALTER USER u2 RENAME TO u3; > ALTER USER > test=> \! cat /u/pg/data/global/pg_group > "g1" "u1" "u3" > > In the patch, notice the old comment that suggests we might need to use > CommandCounterIncrement(). > > This sesms safe to apply to back branches. > > --------------------------------------------------------------------------- > > Dennis Vshivkov wrote: > > Package: postgresql-8.0 > > Version: 8.0.3-13 > > Severity: important > > Tags: patch, upstream > > > > Here's the problem: > > > > db=# CREATE GROUP g1; > > CREATE GROUP > > db=# CREATE USER u1 IN GROUP g1; (1) > > CREATE USER > > > > # cat /var/lib/postgresql/8.0/main/global/pg_group > > # > > > > The file gets rewritten, but the group `g1' line does not get > > added to the file. Continue: > > > > db=# CREATE USER u2 IN GROUP g1; (2) > > CREATE USER > > > > # cat /var/lib/postgresql/8.0/main/global/pg_group > > "g1" "u1" > > # > > > > Now the line is there, but it lacks the latest member. Consider > > this also: > > > > db=# ALTER USER u2 RENAME TO u3; (3) > > ALTER USER > > > > # cat /var/lib/postgresql/8.0/main/global/pg_group > > "g1" "u1" "u2" > > # > > > > The problem is that the code that updates pg_group file resolves > > group membership through the system user catalogue cache. The > > file update happens shortly before the commit, but the caches > > only see updates after the commit. Because of this, new users > > or changes in users' names often do not make it to pg_group. > > That leads to mysterious authentication failures subsequently. > > The problem can also have security implications for certain > > pg_hba.conf arrangements. > > > > The attached `98-6-pg_group-stale-data-fix.patch' makes the code > > in question access the system user table directly and thus fixes > > the cases (1) and (2), however (3) is doubly ill: the user > > renaming code does not even trigger a pg_group file update. > > Hence the other patch, `98-5-rename-user-update-pg_group.patch'. > > > > A byproduct of the main fix is removal of an unlikely system > > cache reference leak which happens if a group member name > > contains a newline. > > > > The problems were found and the fixes were done for PostgreSQL > > 8.0.3 release. The flaws seem intact in 8.0.4 source code, too. > > > > Hope this helps. > > > > -- > > /Awesome Walrus <walrus@amur.ru> > > [ Attachment, skipping... ] > > [ Attachment, skipping... ] > > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 4: Have you searched our list archives? > > > > http://archives.postgresql.org > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 359-1001 > + If your life is a hard drive, | 13 Roberts Road > + Christ can be your backup. | Newtown Square, Pennsylvania 19073 > Index: src/backend/commands/user.c > =================================================================== > RCS file: /cvsroot/pgsql/src/backend/commands/user.c,v > retrieving revision 1.147 > diff -c -c -r1.147 user.c > *** src/backend/commands/user.c 31 Dec 2004 21:59:42 -0000 1.147 > --- src/backend/commands/user.c 14 Oct 2005 15:42:17 -0000 > *************** > *** 175,183 **** > > /* > * Read pg_group and write the file. Note we use SnapshotSelf to > ! * ensure we see all effects of current transaction. (Perhaps could > ! * do a CommandCounterIncrement beforehand, instead?) > */ > scan = heap_beginscan(grel, SnapshotSelf, 0, NULL); > while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) > { > --- 175,183 ---- > > /* > * Read pg_group and write the file. Note we use SnapshotSelf to > ! * ensure we see all effects of current transaction. > */ > + CommandCounterIncrement(); > scan = heap_beginscan(grel, SnapshotSelf, 0, NULL); > while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) > { > *************** > *** 781,786 **** > --- 781,787 ---- > * Set flag to update flat password file at commit. > */ > user_file_update_needed(); > + group_file_update_needed(); > } > > > *************** > *** 1200,1205 **** > --- 1201,1207 ---- > * Set flag to update flat password file at commit. > */ > user_file_update_needed(); > + group_file_update_needed(); > } > > > *************** > *** 1286,1291 **** > --- 1288,1294 ---- > heap_close(rel, NoLock); > > user_file_update_needed(); > + group_file_update_needed(); > } > > > > ---------------------------(end of broadcast)--------------------------- > TIP 6: explain analyze is your friend -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073