Fix incorrect PG_GETARG in pgcrypto
Hi hackers,
I'd like to bring to your attention that I recently identified some
functions in pgcrypto that are using PG_GETARG functions in a way that
doesn't match the expected function signature of the stored
procedures. This patch proposes a solution to address these
inconsistencies and ensure proper alignment.
Thanks,
Shihao
Attachments:
fix_getarg.patchapplication/octet-stream; name=fix_getarg.patchDownload
diff --git a/contrib/pgcrypto/pgp-pgsql.c b/contrib/pgcrypto/pgp-pgsql.c
index 2a6456ab7f..d9b15b07b0 100644
--- a/contrib/pgcrypto/pgp-pgsql.c
+++ b/contrib/pgcrypto/pgp-pgsql.c
@@ -553,15 +553,15 @@ decrypt_internal(int is_pubenc, int need_text, text *data,
Datum
pgp_sym_encrypt_bytea(PG_FUNCTION_ARGS)
{
- bytea *data;
+ bytea *data,
+ *key;
text *arg = NULL;
- text *res,
- *key;
+ text *res;
data = PG_GETARG_BYTEA_PP(0);
- key = PG_GETARG_TEXT_PP(1);
+ key = PG_GETARG_BYTEA_PP(1);
if (PG_NARGS() > 2)
- arg = PG_GETARG_TEXT_PP(2);
+ arg = PG_GETARG_BYTEA_PP(2);
res = encrypt_internal(0, 0, data, key, arg);
@@ -575,15 +575,15 @@ pgp_sym_encrypt_bytea(PG_FUNCTION_ARGS)
Datum
pgp_sym_encrypt_text(PG_FUNCTION_ARGS)
{
- text *data,
+ bytea *data,
*key;
text *arg = NULL;
text *res;
- data = PG_GETARG_TEXT_PP(0);
- key = PG_GETARG_TEXT_PP(1);
+ data = PG_GETARG_BYTEA_PP(0);
+ key = PG_GETARG_BYTEA_PP(1);
if (PG_NARGS() > 2)
- arg = PG_GETARG_TEXT_PP(2);
+ arg = PG_GETARG_BYTEA_PP(2);
res = encrypt_internal(0, 1, data, key, arg);
@@ -598,15 +598,15 @@ pgp_sym_encrypt_text(PG_FUNCTION_ARGS)
Datum
pgp_sym_decrypt_bytea(PG_FUNCTION_ARGS)
{
- bytea *data;
+ bytea *data,
+ *key;
text *arg = NULL;
- text *res,
- *key;
+ text *res;
data = PG_GETARG_BYTEA_PP(0);
- key = PG_GETARG_TEXT_PP(1);
+ key = PG_GETARG_BYTEA_PP(1);
if (PG_NARGS() > 2)
- arg = PG_GETARG_TEXT_PP(2);
+ arg = PG_GETARG_BYTEA_PP(2);
res = decrypt_internal(0, 0, data, key, NULL, arg);
@@ -620,15 +620,15 @@ pgp_sym_decrypt_bytea(PG_FUNCTION_ARGS)
Datum
pgp_sym_decrypt_text(PG_FUNCTION_ARGS)
{
- bytea *data;
+ bytea *data,
+ *key;
text *arg = NULL;
- text *res,
- *key;
+ text *res;
data = PG_GETARG_BYTEA_PP(0);
- key = PG_GETARG_TEXT_PP(1);
+ key = PG_GETARG_BYTEA_PP(1);
if (PG_NARGS() > 2)
- arg = PG_GETARG_TEXT_PP(2);
+ arg = PG_GETARG_BYTEA_PP(2);
res = decrypt_internal(0, 1, data, key, NULL, arg);
@@ -654,7 +654,7 @@ pgp_pub_encrypt_bytea(PG_FUNCTION_ARGS)
data = PG_GETARG_BYTEA_PP(0);
key = PG_GETARG_BYTEA_PP(1);
if (PG_NARGS() > 2)
- arg = PG_GETARG_TEXT_PP(2);
+ arg = PG_GETARG_BYTEA_PP(2);
res = encrypt_internal(1, 0, data, key, arg);
@@ -668,15 +668,15 @@ pgp_pub_encrypt_bytea(PG_FUNCTION_ARGS)
Datum
pgp_pub_encrypt_text(PG_FUNCTION_ARGS)
{
- bytea *key;
+ bytea *data,
+ *key;
text *arg = NULL;
- text *res,
- *data;
+ text *res;
- data = PG_GETARG_TEXT_PP(0);
+ data = PG_GETARG_BYTEA_PP(0);
key = PG_GETARG_BYTEA_PP(1);
if (PG_NARGS() > 2)
- arg = PG_GETARG_TEXT_PP(2);
+ arg = PG_GETARG_BYTEA_PP(2);
res = encrypt_internal(1, 1, data, key, arg);
@@ -700,9 +700,9 @@ pgp_pub_decrypt_bytea(PG_FUNCTION_ARGS)
data = PG_GETARG_BYTEA_PP(0);
key = PG_GETARG_BYTEA_PP(1);
if (PG_NARGS() > 2)
- psw = PG_GETARG_TEXT_PP(2);
+ psw = PG_GETARG_BYTEA_PP(2);
if (PG_NARGS() > 3)
- arg = PG_GETARG_TEXT_PP(3);
+ arg = PG_GETARG_BYTEA_PP(3);
res = decrypt_internal(1, 0, data, key, psw, arg);
@@ -727,9 +727,9 @@ pgp_pub_decrypt_text(PG_FUNCTION_ARGS)
data = PG_GETARG_BYTEA_PP(0);
key = PG_GETARG_BYTEA_PP(1);
if (PG_NARGS() > 2)
- psw = PG_GETARG_TEXT_PP(2);
+ psw = PG_GETARG_BYTEA_PP(2);
if (PG_NARGS() > 3)
- arg = PG_GETARG_TEXT_PP(3);
+ arg = PG_GETARG_BYTEA_PP(3);
res = decrypt_internal(1, 1, data, key, psw, arg);
On Mon, Feb 12, 2024 at 11:30:40PM -0500, shihao zhong wrote:
I'd like to bring to your attention that I recently identified some
functions in pgcrypto that are using PG_GETARG functions in a way that
doesn't match the expected function signature of the stored
procedures. This patch proposes a solution to address these
inconsistencies and ensure proper alignment.
You've indeed grabbed some historical inconsistencies here. Please
note that your patch has reversed diffs (for example, the SQL
definition of pgp_sym_encrypt_bytea uses bytea,text,text as arguments
and your resulting patch shows how HEAD does the job with
bytea,bytea,bytea), but perhaps you have generated it with a command
like `git diff -R`?
--
Michael
On Tue, Feb 13, 2024 at 05:36:36PM +0900, Michael Paquier wrote:
You've indeed grabbed some historical inconsistencies here. Please
note that your patch has reversed diffs (for example, the SQL
definition of pgp_sym_encrypt_bytea uses bytea,text,text as arguments
and your resulting patch shows how HEAD does the job with
bytea,bytea,bytea), but perhaps you have generated it with a command
like `git diff -R`?
The reversed part of the patch put aside aside, I've double-checked
your patch and the inconsistencies seem to be all addressed in this
area.
The symmetry that we have now between the bytea and text versions of
the functions is stunning, but I cannot really get excited about
merging all of them either as it would imply a bump of pgcrypto to
update the prosrc of these functions, and we have to maintain runtime
compatibility with older versions.
--
Michael
On Tue, Feb 13, 2024 at 7:08 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Feb 13, 2024 at 05:36:36PM +0900, Michael Paquier wrote:
You've indeed grabbed some historical inconsistencies here. Please
note that your patch has reversed diffs (for example, the SQL
definition of pgp_sym_encrypt_bytea uses bytea,text,text as arguments
and your resulting patch shows how HEAD does the job with
bytea,bytea,bytea), but perhaps you have generated it with a command
like `git diff -R`?The reversed part of the patch put aside aside, I've double-checked
your patch and the inconsistencies seem to be all addressed in this
area.
Thanks for fixing and merging this patch, I appreciate it!
Thanks,
Shihao
Show quoted text
The symmetry that we have now between the bytea and text versions of
the functions is stunning, but I cannot really get excited about
merging all of them either as it would imply a bump of pgcrypto to
update the prosrc of these functions, and we have to maintain runtime
compatibility with older versions.
--
Michael
On 2/16/24 02:35, shihao zhong wrote:
On Tue, Feb 13, 2024 at 7:08 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Feb 13, 2024 at 05:36:36PM +0900, Michael Paquier wrote:
You've indeed grabbed some historical inconsistencies here. Please
note that your patch has reversed diffs (for example, the SQL
definition of pgp_sym_encrypt_bytea uses bytea,text,text as arguments
and your resulting patch shows how HEAD does the job with
bytea,bytea,bytea), but perhaps you have generated it with a command
like `git diff -R`?The reversed part of the patch put aside aside, I've double-checked
your patch and the inconsistencies seem to be all addressed in this
area.Thanks for fixing and merging this patch, I appreciate it!
Should this be marked as committed, or is there some remaining part?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Feb 26, 2024 at 02:47:27PM +0100, Tomas Vondra wrote:
Should this be marked as committed, or is there some remaining part?
Thanks. I've missed the existence of [1]https://commitfest.postgresql.org/47/4822/ -- Michael. It is now marked as
committed.
[1]: https://commitfest.postgresql.org/47/4822/ -- Michael
--
Michael