public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhihong Yu <[email protected]>
To: Noah Misch <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: null iv parameter passed to combo_init()
Date: Sat, 8 Jan 2022 18:52:14 -0800
Message-ID: <CALNJ-vTh9BXARHg3iaCAzr97Fws2W5aR6nRPJfT4u3VdKDzRTQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CALNJ-vSBb2Ees=KB0frYBh7foK-QNZMbK7Vz66bUJa09D+CHAw@mail.gmail.com>
<[email protected]>
On Sat, Jan 8, 2022 at 5:52 PM Noah Misch <[email protected]> wrote:
> On Fri, Jan 07, 2022 at 04:32:01PM -0800, Zhihong Yu wrote:
> > In contrib/pgcrypto/pgcrypto.c :
> >
> > err = px_combo_init(c, (uint8 *) VARDATA_ANY(key), klen, NULL, 0);
> >
> > Note: NULL is passed as iv.
> >
> > When combo_init() is called,
> >
> > if (ivlen > ivs)
> > memcpy(ivbuf, iv, ivs);
> > else
> > memcpy(ivbuf, iv, ivlen);
> >
> > It seems we need to consider the case of null being passed as iv for
> > memcpy() because of this:
> >
> > /usr/include/string.h:44:28: note: nonnull attribute specified here
>
> I agree it's time to fix cases like this, given
> https://postgr.es/m/flat/[email protected].
> However,
> it should be one patch fixing all (or at least many) of them.
> > --- a/contrib/pgcrypto/px.c
> > +++ b/contrib/pgcrypto/px.c
> > @@ -198,10 +198,13 @@ combo_init(PX_Combo *cx, const uint8 *key,
> unsigned klen,
> > if (ivs > 0)
> > {
> > ivbuf = palloc0(ivs);
> > - if (ivlen > ivs)
> > - memcpy(ivbuf, iv, ivs);
> > - else
> > - memcpy(ivbuf, iv, ivlen);
> > + if (iv != NULL)
> > + {
> > + if (ivlen > ivs)
> > + memcpy(ivbuf, iv, ivs);
> > + else
> > + memcpy(ivbuf, iv, ivlen);
> > + }
> > }
>
> If someone were to pass NULL iv with nonzero ivlen, that will silently
>
Hi,
If iv is NULL, none of the memcpy() would be called (based on my patch).
Can you elaborate your suggestion in more detail ?
Patch v2 is attached, covering more files.
Since the referenced email was old, line numbers have changed.
It would be nice if an up-to-date list is provided in case more places
should be changed.
Cheers
> malfunction. I'd avoid that risk by writing this way:
>
> --- a/contrib/pgcrypto/px.c
> +++ b/contrib/pgcrypto/px.c
> @@ -202,3 +202,3 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned
> klen,
> memcpy(ivbuf, iv, ivs);
> - else
> + else if (ivlen > 0)
> memcpy(ivbuf, iv, ivlen);
>
> That also gives the compiler an additional optimization strategy.
>
Attachments:
[application/octet-stream] 0002-memcpy-null.patch (2.3K, ../CALNJ-vTh9BXARHg3iaCAzr97Fws2W5aR6nRPJfT4u3VdKDzRTQ@mail.gmail.com/3-0002-memcpy-null.patch)
download | inline diff:
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index 4205e9c3ef..d3e057a453 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -198,10 +198,13 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
if (ivs > 0)
{
ivbuf = palloc0(ivs);
- if (ivlen > ivs)
- memcpy(ivbuf, iv, ivs);
- else
- memcpy(ivbuf, iv, ivlen);
+ if (iv != NULL)
+ {
+ if (ivlen > ivs)
+ memcpy(ivbuf, iv, ivs);
+ else
+ memcpy(ivbuf, iv, ivlen);
+ }
}
if (klen > ks)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index de787c3d37..eca1e5dd0e 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -294,7 +294,7 @@ TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
* sub-XIDs and all of the XIDs for which we're adjusting clog should be
* on the same page. Check those conditions, too.
*/
- if (all_xact_same_page && xid == MyProc->xid &&
+ if (subxids != NULL && all_xact_same_page && xid == MyProc->xid &&
nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT &&
nsubxids == MyProc->subxidStatus.count &&
memcmp(subxids, MyProc->subxids.xids,
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c9516e03fa..3d5ca89d42 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5329,7 +5329,7 @@ SerializeTransactionState(Size maxsize, char *start_address)
* of our own, we can just pass along the information that was passed to
* us.
*/
- if (nParallelCurrentXids > 0)
+ if (nParallelCurrentXids > 0 && ParallelCurrentXids != NULL)
{
result->nParallelCurrentXids = nParallelCurrentXids;
memcpy(&result->parallelCurrentXids[0], ParallelCurrentXids,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e5cf1bde13..ae10c55711 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1182,7 +1182,10 @@ DefineIndex(Oid relationId,
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
nparts);
- memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ if (partdesc->oids != NULL)
+ {
+ memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ }
parentDesc = RelationGetDescr(rel);
opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
view thread (4+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: null iv parameter passed to combo_init()
In-Reply-To: <CALNJ-vTh9BXARHg3iaCAzr97Fws2W5aR6nRPJfT4u3VdKDzRTQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox