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 because it 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 because it 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_out i _null_ ));
DATA(insert OID = 1026 ( _filename PGUID -1 -1 f b t \054 0 605 array_in array_out array_in array_out i _null_ ));
DATA(insert OID = 1027 ( _polygon PGUID -1 -1 f b t \054 0 604 array_in array_out array_in array_out d _null_ ));
- /* Note: the size of an aclitem needs to match sizeof(AclItem) in acl.h */
DATA(insert OID = 1033 ( aclitem PGUID 8 -1 f b t \054 0 0 aclitemin aclitemout aclitemin aclitemout i _null_ ));
DESCR("access control list");
DATA(insert OID = 1034 ( _aclitem PGUID -1 -1 f b t \054 0 1033 array_in array_out array_in array_out i _null_ ));
DATA(insert OID = 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_out i _null_ ));
DATA(insert OID = 1026 ( _filename PGUID -1 -1 f b t \054 0 605 array_in array_out array_in array_out i _null_ ));
DATA(insert OID = 1027 ( _polygon PGUID -1 -1 f b t \054 0 604 array_in array_out array_in array_out d _null_ ));
DATA(insert OID = 1033 ( aclitem PGUID 8 -1 f b t \054 0 0 aclitemin aclitemout aclitemin aclitemout i _null_ ));
+ #define ACLITEM_SIZE 8
DESCR("access control list");
DATA(insert OID = 1034 ( _aclitem PGUID -1 -1 f b t \054 0 1033 array_in array_out array_in array_out i _null_ ));
DATA(insert OID = 1040 ( _macaddr PGUID -1 -1 f b t \054 0 829 array_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 is the 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
Import Notes
Reply to msg id not found: YourmessageofTue29Jun1999120157+0900199906290301.MAA24508@ext16.sra.co.jp | Resolved by subject fallback
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, Pennsylvania 19026grant/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
Import Notes
Reply to msg id not found: YourmessageofMon28Jun1999234126-0400.19018.930627686@sss.pgh.pa.us | Resolved by subject fallback
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, Pennsylvania 19026Tatsuo 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
Import Notes
Reply to msg id not found: YourmessageofTue29Jun1999131706+0900199906290417.NAA07267@srapc451.sra.co.jp | Resolved by subject fallback
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
Import Notes
Reply to msg id not found: YourmessageofTue29Jun1999002935-0400.199906290429.AAA03787@candle.pha.pa.us | Resolved by subject fallback
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, Pennsylvania 19026Bruce 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
Import Notes
Reply to msg id not found: YourmessageofTue29Jun1999010733-0400199906290507.BAA05326@candle.pha.pa.us | Resolved by subject fallback
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, Pennsylvania 19026Bruce 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
Show quoted text
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
Import Notes
Reply to msg id not found: YourmessageofWed7Jul1999220641-0400199907080206.WAA19858@candle.pha.pa.us | Resolved by subject fallback
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, Pennsylvania 19026OK, 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
Import Notes
Reply to msg id not found: YourmessageofThu08Jul1999233253-0400.199907090332.XAA06421@candle.pha.pa.us | Resolved by subject fallback