Thread: acl problem in NetBSD/m68k
grant/revoke does not work in NetBSD/m68k. This is due to the wrong
assumption that sizeof(AclItem) is equal to 8 in all platforms. I am
going to fix this by replacing all occurrence of sizeof(AclItem) to
ACLITEM_SIZE (newly defined as 8 in catalog/pg_type.h). See included
patches. If there's no objection, I will commit them. Comments?
---
Tatsuo Ishii
--------------------------- cut here ----------------------------------
*** pgsql/src/backend/utils/adt/acl.c~ Wed May 26 01:11:49 1999
--- pgsql/src/backend/utils/adt/acl.c Tue Jun 29 09:18:18 1999
***************
*** 235,241 **** if (!s) elog(ERROR, "aclitemin: null string");
! aip = (AclItem *) palloc(sizeof(AclItem)); if (!aip) elog(ERROR, "aclitemin: palloc failed"); s =
aclparse(s,aip, &modechg);
--- 235,241 ---- if (!s) elog(ERROR, "aclitemin: null string");
! aip = (AclItem *) palloc(ACLITEM_SIZE); if (!aip) elog(ERROR, "aclitemin: palloc failed"); s =
aclparse(s,aip, &modechg);
***************
*** 445,460 **** { /* end */ memmove((char *) new_aip,
(char*) old_aip,
! num * sizeof(AclItem)); } else { /* middle */
memmove((char *) new_aip, (char *) old_aip,
! dst * sizeof(AclItem)); memmove((char *) (new_aip + dst + 1),
(char*) (old_aip + dst),
! (num - dst) * sizeof(AclItem)); } new_aip[dst].ai_id = mod_aip->ai_id;
new_aip[dst].ai_idtype= mod_aip->ai_idtype;
--- 445,460 ---- { /* end */ memmove((char *) new_aip,
(char*) old_aip,
! num * ACLITEM_SIZE); } else { /* middle */
memmove((char *) new_aip, (char *) old_aip,
! dst * ACLITEM_SIZE); memmove((char *) (new_aip + dst + 1), (char
*)(old_aip + dst),
! (num - dst) * ACLITEM_SIZE); } new_aip[dst].ai_id = mod_aip->ai_id;
new_aip[dst].ai_idtype= mod_aip->ai_idtype;
***************
*** 493,499 **** } ARR_DIMS(new_acl)[0] = num - 1; /* Adjust also the array size
becauseit is used for memmove */
! ARR_SIZE(new_acl) -= sizeof(AclItem); break; } }
--- 493,499 ---- } ARR_DIMS(new_acl)[0] = num - 1; /* Adjust also the array size
becauseit is used for memmove */
! ARR_SIZE(new_acl) -= ACLITEM_SIZE; break; } }
***************
*** 556,571 **** { /* end */ memmove((char *) new_aip,
(char*) old_aip,
! new_num * sizeof(AclItem)); } else { /* middle */
memmove((char *) new_aip, (char *) old_aip,
! dst * sizeof(AclItem)); memmove((char *) (new_aip + dst), (char
*)(old_aip + dst + 1),
! (new_num - dst) * sizeof(AclItem)); } } return new_acl;
--- 556,571 ---- { /* end */ memmove((char *) new_aip,
(char*) old_aip,
! new_num * ACLITEM_SIZE); } else { /* middle */
memmove((char *) new_aip, (char *) old_aip,
! dst * ACLITEM_SIZE); memmove((char *) (new_aip + dst), (char *)
(old_aip+ dst + 1),
! (new_num - dst) * ACLITEM_SIZE); } } return new_acl;
***************
*** 682,688 **** ChangeACLStmt *n = makeNode(ChangeACLStmt); char str[MAX_PARSE_BUFFER];
! n->aclitem = (AclItem *) palloc(sizeof(AclItem)); /* the grantee string is "G <group_name>", "U
<user_name>",or "ALL" */ if (grantee[0] == 'G') /* group permissions */
--- 682,688 ---- ChangeACLStmt *n = makeNode(ChangeACLStmt); char str[MAX_PARSE_BUFFER];
! n->aclitem = (AclItem *) palloc(ACLITEM_SIZE); /* the grantee string is "G <group_name>", "U <user_name>",
or"ALL" */ if (grantee[0] == 'G') /* group permissions */
*** pgsql/src/include/catalog/pg_type.h~ Wed May 26 01:13:48 1999
--- pgsql/src/include/catalog/pg_type.h Tue Jun 29 09:13:46 1999
***************
*** 341,348 **** DATA(insert OID = 1025 ( _tinterval PGUID -1 -1 f b t \054 0 704 array_in array_out array_in
array_outi _null_ )); DATA(insert OID = 1026 ( _filename PGUID -1 -1 f b t \054 0 605 array_in array_out array_in
array_outi _null_ )); DATA(insert OID = 1027 ( _polygon PGUID -1 -1 f b t \054 0 604 array_in array_out array_in
array_outd _null_ ));
- /* Note: the size of an aclitem needs to match sizeof(AclItem) in acl.h */ DATA(insert OID = 1033 ( aclitem
PGUID8 -1 f b t \054 0 0 aclitemin aclitemout aclitemin aclitemout i _null_ )); DESCR("access control list");
DATA(insertOID = 1034 ( _aclitem PGUID -1 -1 f b t \054 0 1033 array_in array_out array_in array_out i _null_ ));
DATA(insertOID = 1040 ( _macaddr PGUID -1 -1 f b t \054 0 829 array_in array_out array_in array_out i _null_ ));
--- 341,348 ---- DATA(insert OID = 1025 ( _tinterval PGUID -1 -1 f b t \054 0 704 array_in array_out array_in
array_outi _null_ )); DATA(insert OID = 1026 ( _filename PGUID -1 -1 f b t \054 0 605 array_in array_out array_in
array_outi _null_ )); DATA(insert OID = 1027 ( _polygon PGUID -1 -1 f b t \054 0 604 array_in array_out array_in
array_outd _null_ )); DATA(insert OID = 1033 ( aclitem PGUID 8 -1 f b t \054 0 0 aclitemin aclitemout aclitemin
aclitemouti _null_ ));
+ #define ACLITEM_SIZE 8 DESCR("access control list"); DATA(insert OID = 1034 ( _aclitem PGUID -1 -1 f b t \054 0
1033array_in array_out array_in array_out i _null_ )); DATA(insert OID = 1040 ( _macaddr PGUID -1 -1 f b t \054 0
829array_in array_out array_in array_out i _null_ ));
*** pgsql/src/include/utils/acl.h~ Sun Feb 14 08:22:14 1999
--- pgsql/src/include/utils/acl.h Tue Jun 29 09:17:40 1999
***************
*** 24,29 ****
--- 24,30 ---- #include <nodes/parsenodes.h> #include <utils/array.h>
+ #include <catalog/pg_type.h> /* * AclId system identifier for the user, group, etc.
***************
*** 79,84 ****
--- 80,92 ---- /* Note: if the size of AclItem changes, change the aclitem typlen in pg_type.h */
+ /* There used to be a wrong assumption that sizeof(AclItem) was
+ always same in all platforms.
+ Of course this is not true for certain platform (for example
+ NetBSD/m68k). For now we use ACLITEM_SIZE defined in catalog/pg_type.h
+ instead of sizeof(AclItem) -- 1999/6/29 Tatsuo
+ */
+ /* * The value of the first dimension-array element. Since these arrays * always have a lower-bound of 0, this
isthe same as the number of
***************
*** 94,100 **** #define ACL_NUM(ACL) ARR_DIM0(ACL) #define ACL_DAT(ACL) ((AclItem *)
ARR_DATA_PTR(ACL))#define ACL_N_SIZE(N) \
! ((unsigned) (ARR_OVERHEAD(1) + ((N) * sizeof(AclItem)))) #define ACL_SIZE(ACL) ARR_SIZE(ACL) /*
--- 102,108 ---- #define ACL_NUM(ACL) ARR_DIM0(ACL) #define ACL_DAT(ACL) ((AclItem *)
ARR_DATA_PTR(ACL))#define ACL_N_SIZE(N) \
! ((unsigned) (ARR_OVERHEAD(1) + ((N) * ACLITEM_SIZE))) #define ACL_SIZE(ACL) ARR_SIZE(ACL) /*
Tatsuo Ishii <t-ishii@sra.co.jp> writes:
> grant/revoke does not work in NetBSD/m68k. This is due to the wrong
> assumption that sizeof(AclItem) is equal to 8 in all platforms. I am
> going to fix this by replacing all occurrence of sizeof(AclItem) to
> ACLITEM_SIZE (newly defined as 8 in catalog/pg_type.h). See included
> patches. If there's no objection, I will commit them. Comments?
I do not like this patch at *all*. Why is sizeof(AclItem) not the
correct thing to use? Replacing it with a hardwired "8" seems like
a step backwards --- not to mention a direct contradiction of what
you claim the patch is doing.
Perhaps the real problem is that the AclItem struct definition needs
modification? Or maybe we need a way to put a machine-dependent size
into the pg_type entry for type aclitem? The latter seems like a
good thing to be able to do on general principles.
regards, tom lane
> Tatsuo Ishii <t-ishii@sra.co.jp> writes: > > grant/revoke does not work in NetBSD/m68k. This is due to the wrong > > assumption that sizeof(AclItem) is equal to 8 in all platforms. I am > > going to fix this by replacing all occurrence of sizeof(AclItem) to > > ACLITEM_SIZE (newly defined as 8 in catalog/pg_type.h). See included > > patches. If there's no objection, I will commit them. Comments? > > I do not like this patch at *all*. Why is sizeof(AclItem) not the > correct thing to use? Replacing it with a hardwired "8" seems like > a step backwards --- not to mention a direct contradiction of what > you claim the patch is doing. > > Perhaps the real problem is that the AclItem struct definition needs > modification? Or maybe we need a way to put a machine-dependent size > into the pg_type entry for type aclitem? The latter seems like a > good thing to be able to do on general principles. This makes a lot of sense. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
>> grant/revoke does not work in NetBSD/m68k. This is due to the wrong >> assumption that sizeof(AclItem) is equal to 8 in all platforms. I am >> going to fix this by replacing all occurrence of sizeof(AclItem) to >> ACLITEM_SIZE (newly defined as 8 in catalog/pg_type.h). See included >> patches. If there's no objection, I will commit them. Comments? > >I do not like this patch at *all*. Why is sizeof(AclItem) not the >correct thing to use? In NetBSD/m68k sizeof(AclItem) = 6, not 8. >Replacing it with a hardwired "8" seems like >a step backwards --- not to mention a direct contradiction of what >you claim the patch is doing. It's already hard wired in pg_type.h, isn't it. >Perhaps the real problem is that the AclItem struct definition needs >modification? Or maybe we need a way to put a machine-dependent size >into the pg_type entry for type aclitem? The latter seems like a >good thing to be able to do on general principles. Glad to hear you have better idea. Anyway, NetBSD/m68k users need some way to fix the problem now, since the problem seems very serious. -- Tatsuo Ishii
One item on this. Let's try to get a proper fix that does not require an initdb for 6.5.1 for m68 users. > >> grant/revoke does not work in NetBSD/m68k. This is due to the wrong > >> assumption that sizeof(AclItem) is equal to 8 in all platforms. I am > >> going to fix this by replacing all occurrence of sizeof(AclItem) to > >> ACLITEM_SIZE (newly defined as 8 in catalog/pg_type.h). See included > >> patches. If there's no objection, I will commit them. Comments? > > > >I do not like this patch at *all*. Why is sizeof(AclItem) not the > >correct thing to use? > > In NetBSD/m68k sizeof(AclItem) = 6, not 8. > > >Replacing it with a hardwired "8" seems like > >a step backwards --- not to mention a direct contradiction of what > >you claim the patch is doing. > > It's already hard wired in pg_type.h, isn't it. > > >Perhaps the real problem is that the AclItem struct definition needs > >modification? Or maybe we need a way to put a machine-dependent size > >into the pg_type entry for type aclitem? The latter seems like a > >good thing to be able to do on general principles. > > Glad to hear you have better idea. Anyway, NetBSD/m68k users need some > way to fix the problem now, since the problem seems very serious. > -- > Tatsuo Ishii > > -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Tatsuo Ishii <t-ishii@sra.co.jp> writes:
>> I do not like this patch at *all*. Why is sizeof(AclItem) not the
>> correct thing to use?
> In NetBSD/m68k sizeof(AclItem) = 6, not 8.
Oh, I see: the struct contains int32, uint8, uint8, and so it will
be padded to a multiple of int32's alignment requirement --- which
is 4 most places but only 2 on m68k.
>> Perhaps the real problem is that the AclItem struct definition needs
>> modification? Or maybe we need a way to put a machine-dependent size
>> into the pg_type entry for type aclitem? The latter seems like a
>> good thing to be able to do on general principles.
>
> Glad to hear you have better idea. Anyway, NetBSD/m68k users need some
> way to fix the problem now, since the problem seems very serious.
There are two ways we could attack this: (1) put a "pad" field into
struct AclItem (prolly two uint8s) to try to ensure that compilers
would think it is 8 bytes long, or (2) make the size field for aclitem
in pg_type.h read "sizeof(AclItem)". I think the latter is a better
long-term solution, because it eliminates having to try to guess
what a compiler will do with a struct declaration. But there are
several possible counterarguments:
* It might require patching the scripts that read pg_type.h --- I
am not sure if they'd work unmodified.
* We'd either need to #include acl.h into pg_type.h or push the
declarations for AclItem into some more-widely-used header.
* In theory this would represent an initdb change and couldn't
be applied before 6.6. In practice, Postgres isn't working right
now on any platform where sizeof(AclItem) != 8, so initdb would
*not* be needed for any working installation.
I don't think any of these counterarguments is a big deal, but
maybe someone else will have a different opinion.
regards, tom lane
>One item on this. Let's try to get a proper fix that does not require >an initdb for 6.5.1 for m68 users. Ok. no initdb means we cannot change the length of data type aclitem (currently 8). I will propose another patch soon (probably change the AciItem structure). BTW, I believe Linux/m68k has the same problem. Can someone confirm this? grant insert on table to somebody; \z shows some strange output on NetBSD/m68k. -- Tatsuo Ishii
> There are two ways we could attack this: (1) put a "pad" field into > struct AclItem (prolly two uint8s) to try to ensure that compilers > would think it is 8 bytes long, or (2) make the size field for aclitem > in pg_type.h read "sizeof(AclItem)". I think the latter is a better > long-term solution, because it eliminates having to try to guess > what a compiler will do with a struct declaration. But there are > several possible counterarguments: > > * It might require patching the scripts that read pg_type.h --- I > am not sure if they'd work unmodified. > > * We'd either need to #include acl.h into pg_type.h or push the > declarations for AclItem into some more-widely-used header. > > * In theory this would represent an initdb change and couldn't > be applied before 6.6. In practice, Postgres isn't working right > now on any platform where sizeof(AclItem) != 8, so initdb would > *not* be needed for any working installation. > > I don't think any of these counterarguments is a big deal, but > maybe someone else will have a different opinion. My guess is that we are looking at different solutions for 6.5.1 and 6.6. A good argument for a source tree split. Currently, initdb runs through pg_type.h using sed/awk, so it can't see any of the sizeof() defines. One hokey solution would be to have the compile process run a small C program that dumps out the acl size into a file, and have initdb pick up that. That is a terrible solution, though. I guess we don't have any other 'struct' data types that need to know the size of the struct on a give OS. Maybe padding with an Assert() to make sure it stays at the fixed size we specify is a good solution. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <maillist@candle.pha.pa.us> writes:
>> There are two ways we could attack this: (1) put a "pad" field into
>> struct AclItem (prolly two uint8s) to try to ensure that compilers
>> would think it is 8 bytes long, or (2) make the size field for aclitem
>> in pg_type.h read "sizeof(AclItem)". I think the latter is a better
>> long-term solution, because it eliminates having to try to guess
>> what a compiler will do with a struct declaration.
> Currently, initdb runs through pg_type.h using sed/awk, so it can't
> see any of the sizeof() defines.
Hmm, that does put a bit of a crimp in the idea :-(
> I guess we don't have any other 'struct' data types that need
> to know the size of the struct on a give OS.
Right now I think all the other ones are either single-type structs (eg
point is two float8s, so no padding) or varlena. But this is something
that will come up again, I foresee...
> Maybe padding with an Assert() to make sure it stays at the fixed size
> we specify is a good solution.
I agree, that's probably an OK patch for now. When we have more than
one such type it'll probably be time to bite the bullet and implement
a clean solution.
regards, tom lane
Then <maillist@candle.pha.pa.us> spoke up and said: > > There are two ways we could attack this: (1) put a "pad" field into > > struct AclItem (prolly two uint8s) to try to ensure that compilers > > would think it is 8 bytes long, or (2) make the size field for aclitem > > in pg_type.h read "sizeof(AclItem)". I think the latter is a better > > long-term solution, because it eliminates having to try to guess > > what a compiler will do with a struct declaration. But there are > > several possible counterarguments: > > Currently, initdb runs through pg_type.h using sed/awk, so it can't > see any of the sizeof() defines. One hokey solution would be to have > the compile process run a small C program that dumps out the acl size > into a file, and have initdb pick up that. That is a terrible solution, > though. I guess we don't have any other 'struct' data types that need > to know the size of the struct on a give OS. Maybe padding with an > Assert() to make sure it stays at the fixed size we specify is a good > solution. Perhaps it would be easier to pipe the output of cpp on pg_type.h thru the awk/sed script? This would have the added advantage of making other system-dependent changes to pg_type.h easier. -- ===================================================================== | JAVA must have been developed in the wilds of West Virginia. | | After all, why else would it support only single inheritance?? | ===================================================================== | Finger geek@cmu.edu for my public key. | =====================================================================
Did we ever fix this? > >> grant/revoke does not work in NetBSD/m68k. This is due to the wrong > >> assumption that sizeof(AclItem) is equal to 8 in all platforms. I am > >> going to fix this by replacing all occurrence of sizeof(AclItem) to > >> ACLITEM_SIZE (newly defined as 8 in catalog/pg_type.h). See included > >> patches. If there's no objection, I will commit them. Comments? > > > >I do not like this patch at *all*. Why is sizeof(AclItem) not the > >correct thing to use? > > In NetBSD/m68k sizeof(AclItem) = 6, not 8. > > >Replacing it with a hardwired "8" seems like > >a step backwards --- not to mention a direct contradiction of what > >you claim the patch is doing. > > It's already hard wired in pg_type.h, isn't it. > > >Perhaps the real problem is that the AclItem struct definition needs > >modification? Or maybe we need a way to put a machine-dependent size > >into the pg_type entry for type aclitem? The latter seems like a > >good thing to be able to do on general principles. > > Glad to hear you have better idea. Anyway, NetBSD/m68k users need some > way to fix the problem now, since the problem seems very serious. > -- > Tatsuo Ishii > > -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Did we ever fix this?
We agreed what to do: add padding field(s) to struct AclItem and
add an Assert() somewhere that would check that sizeof(AclItem) is 8,
while leaving the bulk of the code using sizeof(AclItem) rather than
a #define constant.
But it doesn't look like it got done yet.
regards, tom lane
>>>>> grant/revoke does not work in NetBSD/m68k. This is due to the wrong
>>>>> assumption that sizeof(AclItem) is equal to 8 in all platforms. I am
>>>>> going to fix this by replacing all occurrence of sizeof(AclItem) to
>>>>> ACLITEM_SIZE (newly defined as 8 in catalog/pg_type.h). See included
>>>>> patches. If there's no objection, I will commit them. Comments?
>>>>
>>>> I do not like this patch at *all*. Why is sizeof(AclItem) not the
>>>> correct thing to use?
>>
>> In NetBSD/m68k sizeof(AclItem) = 6, not 8.
>>
>>>> Replacing it with a hardwired "8" seems like
>>>> a step backwards --- not to mention a direct contradiction of what
>>>> you claim the patch is doing.
>>
>> It's already hard wired in pg_type.h, isn't it.
>>
>>>> Perhaps the real problem is that the AclItem struct definition needs
>>>> modification? Or maybe we need a way to put a machine-dependent size
>>>> into the pg_type entry for type aclitem? The latter seems like a
>>>> good thing to be able to do on general principles.
>>
>> Glad to hear you have better idea. Anyway, NetBSD/m68k users need some
>> way to fix the problem now, since the problem seems very serious.
>> --
>> Tatsuo Ishii
> Bruce Momjian <maillist@candle.pha.pa.us> writes: > > Did we ever fix this? > > We agreed what to do: add padding field(s) to struct AclItem and > add an Assert() somewhere that would check that sizeof(AclItem) is 8, > while leaving the bulk of the code using sizeof(AclItem) rather than > a #define constant. > > But it doesn't look like it got done yet. OK, I have added the needed padding, and added an Assert, with comments. Tatsuo, can you check the problem platform please? -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
>OK, I have added the needed padding, and added an Assert, with comments. > >Tatsuo, can you check the problem platform please? Thanks. I should have done it myself, but didn't have time for it. Please let me know when you finish the job. I will check on a NetBSD/m68 machine. -- Tatsuo Ishii
>OK, I have added the needed padding, and added an Assert, with comments. > >Tatsuo, can you check the problem platform please? Thanks. I should have done it myself, but didn't have time for it. Please let me know when you finish the job. I will check on a NetBSD/m68 machine. -- Tatsuo Ishii