--lrZ03NoBR/3+SXJZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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 (AT) amur (DOT) ru>

--lrZ03NoBR/3+SXJZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="98-5-rename-user-update-pg_group.patch"

--- build-tree/postgresql-8.0.3/src/backend/commands/user.c.orig 2005-05-31 14:55:27.000000000 +1200
+++ build-tree/postgresql-8.0.3/src/backend/commands/user.c 2005-05-31 14:55:30.000000000 +1200
@@ -1286,6 +1286,7 @@ RenameUser(const char *oldname, const ch
heap_close(rel, NoLock);

user_file_update_needed();
+ group_file_update_needed();
}



--lrZ03NoBR/3+SXJZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="98-6-pg_group-stale-data-fix.patch"

--- build-tree/postgresql-8.0.3/src/backend/commands/user.c.orig 2005-10-14 11:00:16.000000000 +1300
+++ build-tree/postgresql-8.0.3/src/backend/commands/user.c 2005-10-14 11:01:09.000000000 +1300
@@ -17,6 +17,7 @@
#include <errno.h>
#include <unistd.h>

+#include "access/genam.h"
#include "access/heapam.h"
#include "catalog/catname.h"
#include "catalog/indexing.h"
@@ -154,6 +155,27 @@
HeapScanDesc scan;
HeapTuple tuple;
TupleDesc dsc = RelationGetDescr(grel);
+ /* Look up user names by user system id. */
+ Relation urel = heap_openr(ShadowRelationName, AccessShareLock),
+ ureli = index_openr(ShadowSysidIndex);
+
+ /* Scan for system id equality. */
+ ScanKeyData ukey =
+ {
+ .sk_flags = 0,
+ .sk_attno = 1,
+ .sk_strategy = BTEqualStrategyNumber,
+ .sk_subtype = InvalidOid
+ };
+
+ /* We expect the first index column to be in order. */
+ Assert(Anum_pg_shadow_usesysid == ureli->rd_index->indkey[0]);
+
+ /* And only one system identifier column type. */
+ Assert(RelationGetDescr(urel)->attrs[Anum_pg_shadow_usesysid - 1]->atttypid == INT4OID);
+
+ /* Finish scan key initialisation. */
+ fmgr_info(F_INT4EQ, &ukey.sk_func);

/*
* Create a temporary filename to be renamed later. This prevents the
@@ -192,6 +214,7 @@
num;
char *usename;
bool first_user = true;
+ IndexScanDesc userscan;

datum = heap_getattr(tuple, Anum_pg_group_groname, dsc, &isnull);
/* ignore NULL groupnames --- shouldn't happen */
@@ -221,11 +244,12 @@
/* scan grolist */
num = IDLIST_NUM(grolist_p);
aidp = IDLIST_DAT(grolist_p);
- for (i = 0; i < num; ++i)
+ for (i = 0; i < num; ++i, index_endscan(userscan))
{
- tuple = SearchSysCache(SHADOWSYSID,
- PointerGetDatum(aidp[i]),
- 0, 0, 0);
+ ukey.sk_argument = PointerGetDatum(aidp[i]);
+ userscan = index_beginscan(urel, ureli, SnapshotSelf, 1, &ukey);
+ tuple = index_getnext(userscan, ForwardScanDirection);
+
if (HeapTupleIsValid(tuple))
{
usename = NameStr(((Form_pg_shadow) GETSTRUCT(tuple))->usename);
@@ -254,8 +278,6 @@

first_user = false;
fputs_quote(usename, fp);
-
- ReleaseSysCache(tuple);
}
}
if (!first_user)
@@ -265,6 +287,8 @@
pfree(grolist_p);
}
heap_endscan(scan);
+ index_close(ureli);
+ heap_close(urel, AccessShareLock);

if (FreeFile(fp))
ereport(ERROR,

--lrZ03NoBR/3+SXJZ
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

--lrZ03NoBR/3+SXJZ--

Dennis Vshivkov <walrus (AT) amur (DOT) ru> writes:
Good catch.

Wouldn't a CommandCounterIncrement be a much simpler solution?

Since this code is all gone in CVS tip, there's not going to be any way
of beta-testing a large patch ... and there's also not going to be a lot
of support for pushing a large, poorly tested patch into the back
branches.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Tom Lane wrote:
It is pretty clear where we are missing group_file_update_needed() in
user.c. We did not anticipate the group being modified by CREATE USER,
so adding the group_file_update_needed() seems trivial.

If a CommandCounterIncrement() fixes the problem, that also seems like a
trivial addition.

--
Bruce Momjian | http://candle.pha.pa.us
pgman (AT) candle (DOT) 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

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo (AT) postgresql (DOT) org so that your
message can get through to the mailing list cleanly