Uniforms the errors msgs at tuplestore paths
Hi, Like how the commit https://github.com/postgres/postgres/commit/07daca53bfcad59618a9c6fad304e380cc9d2bc1 The are some paths that were missed: -At jsonfuncs.c The errors mgs do not report about the materialize mode. -At plperl.c The function plperl_return_next_internal does not check rsi appropriately. The error about materialize mode required, is missed. -At pl_exec.c The error mgs do not report about the materialize mode -At pltcl.c: The function pltcl_init_tuple_store does not check rsi appropriately. Patch attached. regards, Ranier Vilela v1-0001-uniform-error-msgs-tuplestore.patch Description: Binary data
Re: Uniforms the errors msgs at tuplestore paths
Em dom., 20 de fev. de 2022 às 11:30, Justin Pryzby escreveu: > On Sun, Feb 20, 2022 at 11:12:42AM -0300, Ranier Vilela wrote: > > Like how the commit > > > https://github.com/postgres/postgres/commit/07daca53bfcad59618a9c6fad304e380cc9d2bc1 > > The are some paths that were missed: > > I think these are all unified by the existing tuplestore patch. > https://commitfest.postgresql.org/37/3420/ I can't see: plperl.c pl_exec.c pttcl.c Only jsonfuncs.c, but the error about "materialized mode" is not reported. regards, Ranier Vilela
Re: Uniforms the errors msgs at tuplestore paths
Em dom., 20 de fev. de 2022 às 22:08, Michael Paquier escreveu: > On Sun, Feb 20, 2022 at 11:37:33AM -0300, Ranier Vilela wrote: > > I can't see: > > plperl.c > > pl_exec.c > > pttcl.c > > > > Only jsonfuncs.c, but the error about "materialized mode" is not > reported. > > Melanie has done a nice analysis of all the code paths doing > materialization checks for her patch with SRF functions. Though there > are parts that can be simplified to reduce the differences in check > patterns before doing the overall refactoring, I think that we'd > better keep any discussion related to this topic on the other thread > rather than splitting the effort across more threads. > Fine, I agree. regards, Ranier Vilela
Re: Uniforms the errors msgs at tuplestore paths
Em dom., 20 de fev. de 2022 às 22:45, Ranier Vilela escreveu: > Em dom., 20 de fev. de 2022 às 22:08, Michael Paquier > escreveu: > >> On Sun, Feb 20, 2022 at 11:37:33AM -0300, Ranier Vilela wrote: >> > I can't see: >> > plperl.c >> > pl_exec.c >> > pttcl.c >> > >> > Only jsonfuncs.c, but the error about "materialized mode" is not >> reported. >> >> Melanie has done a nice analysis of all the code paths doing >> materialization checks for her patch with SRF functions. Though there >> are parts that can be simplified to reduce the differences in check >> patterns before doing the overall refactoring, I think that we'd >> better keep any discussion related to this topic on the other thread >> rather than splitting the effort across more threads. >> > Fine, I agree. > Thanks for the commit Michael. regards, Ranier Vilela >
Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
Em sex., 12 de fev. de 2021 às 03:56, Kyotaro Horiguchi < horikyota@gmail.com> escreveu: > At Wed, 10 Feb 2021 20:12:38 -0300, Ranier Vilela > wrote in > > Hi, > > > > Per Coverity. > > > > If xid is a subtransaction, the setup of base snapshot on the top-level > > transaction, > > can be not optional, otherwise a Dereference null return value > > (NULL_RETURNS) can be raised. > > > > Patch suggestion to fix this. > > > > diff --git a/src/backend/replication/logical/reorderbuffer.c > > b/src/backend/replication/logical/reorderbuffer.c > > index 5a62ab8bbc..3c6a81f716 100644 > > --- a/src/backend/replication/logical/reorderbuffer.c > > +++ b/src/backend/replication/logical/reorderbuffer.c > > @@ -2993,8 +2993,8 @@ ReorderBufferSetBaseSnapshot(ReorderBuffer *rb, > > TransactionId xid, > > */ > > txn = ReorderBufferTXNByXid(rb, xid, true, &is_new, lsn, true); > > if (rbtxn_is_known_subxact(txn)) > > - txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, false, > > - NULL, InvalidXLogRecPtr, false); > > + txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, true, > > + NULL, InvalidXLogRecPtr, true); > > Assert(txn->base_snapshot == NULL); > > If the return from the first call is a subtransaction, the second call > always obtain the top transaction. If the top transaction actualy did > not exist, it's rather the correct behavior to cause SEGV, than > creating a bogus rbtxn. THus it is wrong to set create=true and > create_as_top=true. We could change the assertion like Assert (txn && > txn->base_snapshot) to make things clearer. > It does not make sense to generate a SEGV on purpose and assertion would not solve why this happens in a production environment. Better to report an error if the second call fails. What do you suggest as a description of the error? regards, Ranier Vilela
Re: Possible dereference after null check (src/backend/executor/ExecUtils.c)
Em sex., 12 de fev. de 2021 às 03:28, Kyotaro Horiguchi <
horikyota@gmail.com> escreveu:
> At Wed, 10 Feb 2021 19:54:46 -0300, Ranier Vilela
> wrote in
> > Hi,
> >
> > Per Coverity.
> >
> > The functions ExecGetInsertedCols and ExecGetUpdatedCols at ExecUtils.c
> > only are safe to call if the variable "ri_RangeTableIndex" is != 0.
> >
> > Otherwise a possible Dereference after null check (FORWARD_NULL) can be
> > raised.
>
> As it turns out, it's a false positive. And perhaps we don't want to
> take action just to satisfy the static code analyzer.
>
>
> The coment in ExecGetInsertedCols says:
>
> > /*
> > * The columns are stored in the range table entry. If this ResultRelInfo
> > * doesn't have an entry in the range table (i.e. if it represents a
> > * partition routing target), fetch the parent's RTE and map the columns
> > * to the order they are in the partition.
> > */
> > if (relinfo->ri_RangeTableIndex != 0)
> > {
>
> This means that any one of the two is always usable here. AFAICS,
> actually, ri_RangeTableIndex is non-zero for partitioned (=leaf) and
> non-partitoned relations and ri_RootResultRelInfo is non-null for
> partitioned (parent or intermediate) relations (since they don't have
> a coressponding range table entry).
>
> The only cases where both are 0 and NULL are trigger-use, which is
> unrelated to the code path.
>
This is a case where it would be worth an assertion.
What do you think?
regards,
Ranier Vilela
Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
Em sáb., 13 de fev. de 2021 às 01:07, Zhihong Yu escreveu: > Hi, > How about the following patch ? > > ReorderBufferSetBaseSnapshot() can return a bool to indicate whether the > base snapshot is set up. > > For the call by SnapBuildCommitTxn(), it seems xid is top transaction. So > the return value doesn't need to be checked. > IMO anything else is better than PANIC. Anyway, if all fails, reporting an error can contribute to checking where. Attached a patch suggestion v2. 1. SnapBuildProcessChange returns a result of ReorderBufferSetBaseSnapshot, so the caller can act accordingly. 2. SnapBuildCommitTxn can't ignore a result from ReorderBufferSetBaseSnapshot, even if it never fails. regards, Ranier Vilela reorderbuffer.patch Description: Binary data
Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)
Em sex., 12 de fev. de 2021 às 22:47, Michael Paquier escreveu: > On Fri, Feb 12, 2021 at 03:21:40PM +0900, Kyotaro Horiguchi wrote: > > The v3 drops the changes of the uuid_ossp contrib. I'm not sure the > > change of scram_HMAC_final is needed. > > Meaning that v3 would fail to compile uuid-ossp. v3 also produces > compilation warnings in auth-scram.c. > > > In v2, int_md5_finish() calls pg_cryptohash_final() with > > h->block_size(h) (64) but it should be h->result_size(h) > > (16). int_sha1_finish() is wrong the same way. (and, v3 seems fixing > > them in the wrong way.) > > Right. These should just use h->result_size(h), and not > h->block_size(h). > > -extern int scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx); > +extern int scram_HMAC_final(scram_HMAC_ctx *ctx, uint8 *result); > There is no point in this change. You just make back-patching harder > while doing nothing about the problem at hand. > IMO there is no necessity in back-patching. > - if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0) > + if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf, > + PG_SHA256_DIGEST_LENGTH) < 0) > Here this could just use sizeof(checksumbuf)? This pattern could be > used elsewhere as well, like in md5_common.c. > Done. Attached a v4 of patch. regards, Ranier Vilela pg_cryptohash_v4.patch Description: Binary data
Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
Em sáb., 13 de fev. de 2021 às 17:35, Ranier Vilela escreveu: > > Em sáb., 13 de fev. de 2021 às 01:07, Zhihong Yu > escreveu: > >> Hi, >> How about the following patch ? >> >> ReorderBufferSetBaseSnapshot() can return a bool to indicate whether the >> base snapshot is set up. >> >> For the call by SnapBuildCommitTxn(), it seems xid is top transaction. So >> the return value doesn't need to be checked. >> > IMO anything else is better than PANIC. > Anyway, if all fails, reporting an error can contribute to checking where. > > Attached a patch suggestion v2. > Sorry, I forgot to mention, it is based on a patch from Zhihong Yu. regards, Ranier Vilela
Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
Em sáb., 13 de fev. de 2021 às 17:48, Zhihong Yu
escreveu:
> Hi,
> + (errmsg("BaseSnapshot cant't be setup at point %X/%X",
> + (uint32) (lsn >> 32), (uint32) lsn),
> +errdetail("Top transaction is running.")));
>
> Did you mean this errdetail:
>
> Top transaction is not running.
>
Done.
Thanks Zhihong.
v3 based on your patch, attached.
regards,
Ranier Vilela
reorderbuffer_v3.patch
Description: Binary data
Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)
Em sáb., 13 de fev. de 2021 às 20:32, Michael Paquier escreveu: > On Sat, Feb 13, 2021 at 05:37:32PM -0300, Ranier Vilela wrote: > > IMO there is no necessity in back-patching. > > You are missing the point here. What you are proposing here would not > be backpatched. However, reusing the same words as upthread, this has > a cost in terms of *future* maintenance. In short, any *future* > potential bug fix that would require to be backpatched in need of > using this function or touching its area would result in a conflict. > Ok. +1 for back-patching. Any future maintenance, or use of that functions, need to consult the api. scram_HMAC_init(scram_HMAC_ctx *ctx, const uint8 *key, int keylen); scram_HMAC_update(scram_HMAC_ctx *ctx, const char *str, int slen); scram_HMAC_final(uint8 *result, scram_HMAC_ctx *ctx); See both "result" and "ctx" are pointers. Someone can use like this: scram_HMAC_init(&ctx, key, keylen); scram_HMAC_update(&ctx, str, slen); scram_HMAC_final(&ctx, result); // parameters wrong order And many compilers won't complain. regards, Ranier Vilela
Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)
Em dom., 14 de fev. de 2021 às 08:22, Michael Paquier escreveu: > On Sat, Feb 13, 2021 at 09:33:48PM -0300, Ranier Vilela wrote: > > Em sáb., 13 de fev. de 2021 às 20:32, Michael Paquier < > mich...@paquier.xyz> > > escreveu: > > > >> You are missing the point here. What you are proposing here would not > >> be backpatched. However, reusing the same words as upthread, this has > >> a cost in terms of *future* maintenance. In short, any *future* > >> potential bug fix that would require to be backpatched in need of > >> using this function or touching its area would result in a conflict. > > > > Ok. +1 for back-patching. > > Please take the time to read again my previous email. > > And also, please take the time to actually test patches you send, > because the list of things getting broken is impressive. At least > you make sure that the internals of cryptohash.c generate an error as > they should because of those incorrect sizes :) > > git diff --check complains, in various places. > > @@ -330,7 +330,8 @@ SendBackupManifest(backup_manifest_info *manifest) > * twice. > */ > manifest->still_checksumming = false; > - if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0) > + if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf, > + > sizeof(checksumbuf) - 1) < 0) > elog(ERROR, "failed to finalize checksum of backup > manifest"); > This breaks backup manifests, due to an incorrect calculation. > Bad habits. sizeof - 1, I use with strings. > I think that in pg_checksum_final() we had better save the digest > length in "retval" before calling pg_cryptohash_final(), and use it > for the size passed down. > pg_checksum_final I would like to see it like this: case CHECKSUM_TYPE_SHA224: retval = PG_SHA224_DIGEST_LENGTH; break; case CHECKSUM_TYPE_SHA256: retval = PG_SHA256_DIGEST_LENGTH; break; case CHECKSUM_TYPE_SHA384: retval = PG_SHA384_DIGEST_LENGTH; break; case CHECKSUM_TYPE_SHA512: retval = PG_SHA512_DIGEST_LENGTH; break; default: return -1; } if (pg_cryptohash_final(context->raw_context.c_sha2, output, retval) < 0) return -1; pg_cryptohash_free(context->raw_context.c_sha2); What do you think? regards, Ranier Vilela
Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)
Em dom., 14 de fev. de 2021 às 22:28, Michael Paquier escreveu: > On Sun, Feb 14, 2021 at 11:39:47AM -0300, Ranier Vilela wrote: > > What do you think? > > That's not a good idea for two reasons: > 1) There is CRC32 to worry about, which relies on a different logic. > 2) It would become easier to miss the new option as compilation would > not warn anymore if a new checksum type is added. > > I have reviewed my patch this morning, tweaked a comment, and applied > it. > Thanks for the commit. regards, Ranier Vilela
Re: Possible dereference after null check (src/backend/executor/ExecUtils.c)
Em sex., 12 de fev. de 2021 às 13:11, Ranier Vilela
escreveu:
> Em sex., 12 de fev. de 2021 às 03:28, Kyotaro Horiguchi <
> horikyota@gmail.com> escreveu:
>
>> At Wed, 10 Feb 2021 19:54:46 -0300, Ranier Vilela
>> wrote in
>> > Hi,
>> >
>> > Per Coverity.
>> >
>> > The functions ExecGetInsertedCols and ExecGetUpdatedCols at ExecUtils.c
>> > only are safe to call if the variable "ri_RangeTableIndex" is != 0.
>> >
>> > Otherwise a possible Dereference after null check (FORWARD_NULL) can be
>> > raised.
>>
>> As it turns out, it's a false positive. And perhaps we don't want to
>> take action just to satisfy the static code analyzer.
>>
>>
>> The coment in ExecGetInsertedCols says:
>>
>> > /*
>> > * The columns are stored in the range table entry. If this
>> ResultRelInfo
>> > * doesn't have an entry in the range table (i.e. if it represents a
>> > * partition routing target), fetch the parent's RTE and map the columns
>> > * to the order they are in the partition.
>> > */
>> > if (relinfo->ri_RangeTableIndex != 0)
>> > {
>>
>> This means that any one of the two is always usable here. AFAICS,
>> actually, ri_RangeTableIndex is non-zero for partitioned (=leaf) and
>> non-partitoned relations and ri_RootResultRelInfo is non-null for
>> partitioned (parent or intermediate) relations (since they don't have
>> a coressponding range table entry).
>>
>> The only cases where both are 0 and NULL are trigger-use, which is
>> unrelated to the code path.
>>
> This is a case where it would be worth an assertion.
> What do you think?
>
Apparently my efforts with Coverity have been worth it.
And together we are helping to keep Postgres more secure.
Although sometimes it is not recognized for that [1].
regards,
Ranier Vilela
[1]
https://github.com/postgres/postgres/commit/54e51dcde03e5c746e8de6243c69fafdc8d0ec7a
Re: Permission failures with WAL files in 13~ on Windows
>Yeah, it'd definitely be good to figure out exactly what it is that >triggers the issue. I think that this issue is the same at 1. And IMHO the patch solves, but nobody is interested. I think that "MOVEFILE_COPY_ALLOWED" that's what is missing. At least on my machine, Postgres can rename statistics files. regards, Ranier Vilela 1. https://www.postgresql.org/message-id/20200616041018.GR20404%40telsasoft.com fix_windows_rename.patch Description: Binary data
re: crash during cascaded foreign key update
>0 0x7f747e6e2387 in raise () from /lib64/libc.so.6 >#1 0x7f747e6e3a78 in abort () from /lib64/libc.so.6 >#2 0x00ae056a in ExceptionalCondition ( >conditionName=0xb67c10 "!ItemPointerEquals(&oldtup.t_self, >&oldtup.t_data->t_ctid)", >errorType=0xb66d89 "FailedAssertion", fileName=0xb66e68 >"heapam.c", lineNumber=3560) at assert.c:69 >#3 0x004eed16 in heap_update (relation=0x7f747f569590, >otid=0x7ffe6f236ec0, newtup=0x1c214b8, cid=2, >crosscheck=0x1c317f8, wait=true, tmfd=0x7ffe6f236df0, l>ockmode=0x7ffe6f236dec) at heapam.c:3560 I have this report from one static analysis tool: heapam.c (9379): Dereferencing of a potential null pointer 'oldtup.t_data' regards, Ranier Vilela
Dereference before NULL check (src/backend/storage/ipc/latch.c)
Hi, Per Coverity. If test set->latch against NULL, is why it can be NULL. ResetEvent can dereference NULL. regards, Ranier Vilela fix_dereference_before_null_check_latch.patch Description: Binary data
Explicit NULL dereference (src/backend/utils/adt/ruleutils.c)
Hi, Per Coverity. make_ruledef function can dereference a NULL pointer (actions), if "ev_qual" is provided and "actions" does not exist. The comment there is contradictory: " /* these could be nulls */ " Because if "ev_qual" is not null, "actions" cannot be either. Solution proposed merely as a learning experience. regards, Ranier Vilela fix_explicit_null_dereference_ruleutils.patch Description: Binary data
Re: Explicit NULL dereference (src/backend/utils/adt/ruleutils.c)
Em seg., 2 de nov. de 2020 às 01:36, Kyotaro Horiguchi <
horikyota@gmail.com> escreveu:
> At Sun, 01 Nov 2020 21:05:29 -0500, Tom Lane wrote in
> > Kyotaro Horiguchi writes:
> > > We cannot reach there with ev_action == NULL since it comes from a
> > > non-nullable column. Since most of the other columns has an assertion
> > > that !isnull, I think we should do the same thing for ev_action (and
> > > ev_qual). SPI_getvalue() returns C-NULL for SQL-NULL (or for some
> > > other unexpected situations.).
> >
> > Isn't the comment just above there wrong?
> >
> > /* these could be nulls */
> >
> > I wonder just when that became outdated.
>
> Mmm. I investigated that.
>
> At the very beginning of CREATE RULE (d31084e9d1, 1996), InsertRule()
> did the following.
>
> >template = "INSERT INTO pg_rewrite \
> >(rulename, ev_type, ev_class, ev_attr, action, ev_qual, is_instead)
> VALUES \
> >('%s', %d::char, %d::oid, %d::int2, '%s'::text, '%s'::text, \
> > '%s'::bool);";
> >if (strlen(template) + strlen(rulname) + strlen(actionbuf) +
> > strlen(qualbuf) + 20 /* fudge fac */ > RULE_PLAN_SIZE) {
> > elog(WARN, "DefineQueryRewrite: rule plan string too big.");
> >}
> >sprintf(rulebuf, template,
> > rulname, evtype, eventrel_oid, evslot_index, actionbuf,
> > qualbuf, is_instead);
>
> Doesn't seem that ev_qual and ev_action can be NULL. The same
> function in the current converts action list to string using
> nodeToSTring so NIL is converted into '<>', which is not NULL.
>
> So I think ev_action cannot be null from the beginning of the history
> unless the columns is modified manually. ev_qual and ev_action are
> marked as non-nullable (9b39b799db, in 2018). They could be null if we
> modified that columns nullable then set NULL, but that could happen on
> all other columns in pg_rewite catalog, which are Assert(!null)ed.
>
> Although ev_action cannot be a empty list using SQL interface. So we
> can get rid of the case list_length(action) == 0, but I'm not sure
> it's worth doing (but the attaches does..).
>
I think that Assert is not the right solution here.
For a function that returns NULL twice (SPI_getvalue), it is worth testing
the result against NULL.
In the future, any modification may cause further dereference.
In addition, the static analysis tools would continue to note this snippet
either as a bug or as a suspect.
Checking "actions" pointer against NULL, and acting appropriately would do.
regards,
Ranier Vilela
Re: Dereference before NULL check (src/backend/storage/ipc/latch.c)
Em seg., 2 de nov. de 2020 às 05:25, Kyotaro Horiguchi < horikyota@gmail.com> escreveu: > At Mon, 02 Nov 2020 14:33:40 +0900 (JST), Kyotaro Horiguchi < > horikyota@gmail.com> wrote in > > At Mon, 2 Nov 2020 16:22:09 +1300, Thomas Munro > wrote in > > > On Mon, Nov 2, 2020 at 1:49 PM Kyotaro Horiguchi > > > wrote: > > > > At Sat, 31 Oct 2020 11:40:53 -0300, Ranier Vilela < > ranier...@gmail.com> wrote in > > > > > Per Coverity. > > > > > > > > > > If test set->latch against NULL, is why it can be NULL. > > > > > ResetEvent can dereference NULL. > > > > > > > > If the returned event is WL_LATCH_SET, set->latch cannot be NULL. We > > > > shouldn't inadvertently ignore the unexpected or broken situation.We > > > > could put Assert instead, but I think that we don't need do something > > > > here at all since SIGSEGV would be raised at the right location. > > > > > > Hmm. I changed that to support set->latch == NULL, so that you can > > > use the long lived WES in the rare code paths that call WaitLatch() > > > without a latch (for example the code I proposed at [1]). The Windows > > > > Ooo. We don't update epoll events in that case. Ok, I understand > > WL_LATCH_SET can fire while set->latch == NULL. > > > > (I was confused by WaitEventAdjust* asserts set->latch != NULL for > > WL_LATCH_SET. Isn't it better if we moved the check on latch from > > ModifyWaitEvent() to WaitEventAdjust*()?)) > > > > > version leaves the event handle of the most recently used latch in > > > set->handles[n] (because AFAICS there is no way to have a "hole" in > > > the handles array). The event can fire while you are waiting on "no > > > latch". Perhaps it should be changed to > > > ResetEvent(set->handles[cur_event->pos + 1])? > > > > > > [1] > https://www.postgresql.org/message-id/flat/CA%2BhUKGK1607VmtrDUHQXrsooU%3Dap4g4R2yaoByWOOA3m8xevUQ%40mail.gmail.com > > > > Seems right. Just doing that *seems* to work fine, but somehow I > > cannot build on Windows for now... > > That was caused by a leftover change on config_default.pl I made when > I tried to enable NLS. > > I called SetLatch() during WaitLatch(NULL, ) but that doesn't fire > WL_LATCH_SET event for me on Windows. (I got it fired on Linux..) On > Windows, the latch is detected after exiting the WaitLatch() > call. Seems like MyLatch of waiter is different from > peerPGPROC->procLatch. And... an update for Visual Studio broke my > environment... I will investigate this further but everything feel > cumbersome on Windows... > I can build. vc_regress is it enough to test it? regards, Ranier Vilela
Re: Dereference before NULL check (src/backend/storage/ipc/latch.c)
Em ter., 3 de nov. de 2020 às 22:09, Kyotaro Horiguchi < horikyota@gmail.com> escreveu: > At Tue, 3 Nov 2020 20:44:23 +1300, Thomas Munro > wrote in > > On Tue, Nov 3, 2020 at 12:50 AM Kyotaro Horiguchi > > wrote: > > > With the fix patch, it changes to: > > > > > > [16632] LOG: FALSE LATCH: > > > > Nice repo. But is it OK to not reset the Win32 event in this case? > > Does it still work correctly if you wait on the latch after that > > happened, and perhaps after the PG latch is reset? > > I'm not sure what is the point of the question, but the previous patch > doesn't omit resetting the Win32-event in that case. In the same way > with other implements of the same function, it resets the trigger that > woke up the process since the trigger is no longer needed even if we > are not waiting on it. > > If we call WaitLatch(OrSocket) that waits on the latch, it immediately > returns because the latch is set. If we called ResetLatch before the > next call to WaitLatch(), it correctly waits on a trigger to be > pulled. > +1 The patch for me is syntactically equal to the code changed and avoids the dereference. regards, Ranier Vilela
re: pgbench stopped supporting large number of client connections on Windows
Hi Marina,
Nice catch.
>rc/bin/pgbench/pgbench.c, the function add_socket_to_set:
>if (fd < 0 || fd >= FD_SETSIZE)
>{
>/*
>* Doing a hard exit here is a bit grotty, but it doesn't seem worth
>* complicating the API to make it less grotty.
>*/
>pg_log_fatal("too many client connections for select()");
>exit(1);
>}
It seems to me that the limit is hardcode in,
src/backend/port/win32/socket.c
FD_SETSIZE * 2
that would be 2048?
regards,
Ranier Vilela
Re: pgbench stopped supporting large number of client connections on Windows
Em sáb., 7 de nov. de 2020 às 14:55, Marina Polyakova < m.polyak...@postgrespro.ru> escreveu: > On 2020-11-06 23:54, Ranier Vilela wrote: > > Hi Marina, > > Hello! > > 1) If you mean the function pgwin32_select in the file > src/backend/port/win32/socket.c, IIUC it is only used in the backend, > see src/include/port/win32_port.h: > > #ifndef FRONTEND > <...> > #define select(n, r, w, e, timeout) pgwin32_select(n, r, w, e, timeout) > <...> > #endif /* FRONTEND */ > Yes. My mistake, you right here. > 2) It looks like FD_SETSIZE does not set a limit on the socket value on > Windows, see > > https://docs.microsoft.com/en-us/windows/win32/winsock/maximum-number-of-sockets-supported-2 > : > > The maximum number of sockets that a Windows Sockets application can use > is not affected by the manifest constant FD_SETSIZE. This value defined > in the Winsock2.h header file is used in constructing the FD_SET > structures used with select function. > Correct. It seems that the limit will be defined by compilation, before the inclusion of Winsock2.h. Have you tried to define -DFD_SETSIZE=2048 best regards, Ranier Vilela
Re: Windows regress fails (latest HEAD)
Em ter., 10 de nov. de 2020 às 14:16, Russell Foster < russell.foster.cod...@gmail.com> escreveu: > On Tue, Nov 10, 2020 at 12:03 PM Ranier Vilela > wrote: > > > > Em sex., 26 de jun. de 2020 às 08:21, Ranier Vilela > escreveu: > >> > >> Em qui., 11 de jun. de 2020 às 10:28, Ranier Vilela < > ranier...@gmail.com> escreveu: > >>> > >>> Em qui., 11 de jun. de 2020 às 10:01, Andrew Dunstan < > andrew.duns...@2ndquadrant.com> escreveu: > >>>> > >>>> > >>>> On 6/11/20 8:52 AM, Ranier Vilela wrote: > >>>> > Hi, > >>>> > Latest HEAD, fails with windows regress tests. > >>>> > > >>>> > float8 ... FAILED 517 ms > >>>> > partition_prune ... FAILED 3085 ms > >>>> > > >>>> > > >>>> > >>>> The first thing you should do when you find this is to see if there > is a > >>>> buildfarm report of the failure. If there isn't then try to work out > why. > >>> > >>> Sorry, I will have to research the buildfarm, I have no reference to > it. > >>> > >>>> > >>>> > >>>> Also, when making a report like this, it is essential to let us know > >>>> things like: > >>>> > >>>> * which commit is causing the failure (git bisect is good for > finding > >>>> this) > >>> > >>> Thanks for hit (git bisect). > >>> > >>>> > >>>> * what Windows version you're testing on > >>> > >>> Windows 10 (2004) > >>> > >>>> * which compiler you're using > >>> > >>> msvc 2019 (64 bits) > >> > >> > >> Only for registry, if anyone else is using msvc 2019. > >> I'm using latest msvc 2019 64 bits (16.6.0) > >> Problably this is a compiler optimization bug. > >> vcregress check with build DEBUG, pass all 200 tests. > > > > With the current HEAD, the regression float8 in release mode (msvc 2019 > 64 bits) is gone. > > Maybe it's this commit: > > > https://github.com/postgres/postgres/commit/0aa8f764088ea0f36620ae2955fa6c54ec736c46 > > > > But (partition_prune) persists. > > partition_prune ... FAILED > > > > regards, > > Ranier Vilela > > I am also experiencing this issue on one of my Windows machines (x64) > using 12.4. I believe this is new, possibly since 12.2. It doesn't > occur on another machine though, which is strange. It appears to be > the same diff output. Is it possible that the given result is also > valid for this test? > Hi Russel, In DEBUG mode, the issue is gone (all 202 tests pass). You can be sure yourself. I think that compiler code generation bug... Ranier Vilela
pg_cryptohash_final possible out-of-bounds access (per Coverity)
Hi Hackers,
Per Coverity.
Coverity complaints about pg_cryptohash_final function.
And I agree with Coverity, it's a bad design.
Its allows this:
#define MY_RESULT_LENGTH 32
function pgtest(char * buffer, char * text) {
pg_cryptohash_ctx *ctx;
uint8 digest[MY_RESULT_LENGTH];
ctx = pg_cryptohash_create(PG_SHA512);
pg_cryptohash_init(ctx);
pg_cryptohash_update(ctx, (uint8 *) buffer, text);
pg_cryptohash_final(ctx, digest); // <-- CID 1446240 (#1 of 1):
Out-of-bounds access (OVERRUN)
pg_cryptohash_free(ctx);
return
}
Attached has a patch with suggestions to make things better.
regards,
Ranier Vilela
pg_cryptohash.patch
Description: Binary data
Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)
Em qua., 10 de fev. de 2021 às 01:44, Kyotaro Horiguchi <
horikyota@gmail.com> escreveu:
> At Tue, 9 Feb 2021 22:01:45 -0300, Ranier Vilela
> wrote in
> > Hi Hackers,
> >
> > Per Coverity.
> >
> > Coverity complaints about pg_cryptohash_final function.
> > And I agree with Coverity, it's a bad design.
> > Its allows this:
> >
> > #define MY_RESULT_LENGTH 32
> >
> > function pgtest(char * buffer, char * text) {
> > pg_cryptohash_ctx *ctx;
> > uint8 digest[MY_RESULT_LENGTH];
> >
> > ctx = pg_cryptohash_create(PG_SHA512);
> > pg_cryptohash_init(ctx);
> > pg_cryptohash_update(ctx, (uint8 *) buffer, text);
> > pg_cryptohash_final(ctx, digest); // <-- CID 1446240 (#1 of 1):
> > Out-of-bounds access (OVERRUN)
> > pg_cryptohash_free(ctx);
> > return
> > }
>
> It seems to me that the above just means the caller must provide a
> digest buffer that fits the use. In the above example digest just must
> be 64 byte. If Coverity complains so, what should do for the
> complaint is to fix the caller to provide a digest buffer of the
> correct size.
>
Exactly.
> Could you show the detailed context where Coverity complained?
>
Coverity complains about call memcpy with fixed size, in a context with
buffer variable size supplied by the caller.
>
> > Attached has a patch with suggestions to make things better.
>
> So it doesn't seem to me the right direction. Even if we are going to
> make pg_cryptohash_final to take the buffer length, it should
> error-out or assert-out if the length is too small rather than copy a
> part of the digest bytes. (In short, it would only be assertion-use.)
>
It is necessary to correct the interfaces. To caller, inform the size of
the buffer it created.
I think it should be error-out, because the buffer can be malloc.
regards,
Ranier Vilela
Possible dereference after null check (src/backend/executor/ExecUtils.c)
Hi,
Per Coverity.
The functions ExecGetInsertedCols and ExecGetUpdatedCols at ExecUtils.c
only are safe to call if the variable "ri_RangeTableIndex" is != 0.
Otherwise a possible Dereference after null check (FORWARD_NULL) can be
raised.
Exemple:
void
1718ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
1719TupleTableSlot *
slot,
1720EState *estate)
1721{
1722Oid root_relid;
1723TupleDesc tupdesc;
1724char *val_desc;
1725Bitmapset *modifiedCols;
1726
1727/*
1728
* If the tuple has been routed, it's been converted to the partition's
1729
* rowtype, which might differ from the root table's. We must
convert it
1730
* back to the root table's rowtype so that val_desc in the
error message
1731 * matches the input tuple.
1732 */
1. Condition resultRelInfo->ri_RootResultRelInfo, taking false branch.
2. var_compare_op: Comparing resultRelInfo->ri_RootResultRelInfo to null
implies that resultRelInfo->ri_RootResultRelInfo might be null.
1733if (resultRelInfo->ri_RootResultRelInfo)
1734{
1735ResultRelInfo *rootrel = resultRelInfo->
ri_RootResultRelInfo;
1736TupleDesc old_tupdesc;
1737AttrMap*map;
1738
1739root_relid = RelationGetRelid(rootrel->ri_RelationDesc);
1740tupdesc = RelationGetDescr(rootrel->ri_RelationDesc);
1741
1742old_tupdesc = RelationGetDescr(resultRelInfo->
ri_RelationDesc);
1743/* a reverse map */
1744map = build_attrmap_by_name_if_req(old_tupdesc, tupdesc
);
1745
1746/*
1747
* Partition-specific slot's tupdesc can't be changed,
so allocate a
1748 * new one.
1749 */
1750if (map != NULL)
1751slot = execute_attr_map_slot(map, slot,
1752
MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
1753modifiedCols = bms_union(ExecGetInsertedCols(rootrel,
estate),
1754
ExecGetUpdatedCols(rootrel, estate));
1755}
1756else
1757{
1758root_relid = RelationGetRelid(resultRelInfo->
ri_RelationDesc);
1759tupdesc = RelationGetDescr(resultRelInfo->
ri_RelationDesc);
CID 1446241 (#1 of 1): Dereference after null check (FORWARD_NULL)3.
var_deref_model: Passing resultRelInfo to ExecGetInsertedCols, which
dereferences null resultRelInfo->ri_RootResultRelInfo. [show details
]
1760modifiedCols = bms_union(ExecGetInsertedCols(
resultRelInfo, estate),
1761
ExecGetUpdatedCols(resultRelInfo, estate));
1762}
1763
1764val_desc = ExecBuildSlotValueDescription(root_relid,
1765
slot,
1766
tupdesc,
1767
modifiedCols,
1768
64);
1769ereport(ERROR,
1770(errcode(ERRCODE_CHECK_VIOLATION),
1771 errmsg(
"new row for relation \"%s\" violates partition constraint",
1772
RelationGetRelationName(resultRelInfo->ri_RelationDesc)),
1773 val_desc ? errdetail("Failing row contains %s."
, val_desc) : 0,
1774 errtable(resultRelInfo->ri_RelationDesc)));
1775}
regards,
Ranier Viela
Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
Hi, Per Coverity. If xid is a subtransaction, the setup of base snapshot on the top-level transaction, can be not optional, otherwise a Dereference null return value (NULL_RETURNS) can be raised. Patch suggestion to fix this. diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 5a62ab8bbc..3c6a81f716 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2993,8 +2993,8 @@ ReorderBufferSetBaseSnapshot(ReorderBuffer *rb, TransactionId xid, */ txn = ReorderBufferTXNByXid(rb, xid, true, &is_new, lsn, true); if (rbtxn_is_known_subxact(txn)) - txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, false, - NULL, InvalidXLogRecPtr, false); + txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, true, + NULL, InvalidXLogRecPtr, true); Assert(txn->base_snapshot == NULL); txn->base_snapshot = snap; regards, Ranier Vilela reorderbuffer.patch Description: Binary data
Operands don't affect result (CONSTANT_EXPRESSION_RESULT) (src/backend/utils/adt/jsonfuncs.c)
Hi, Per Coverity. The use of type "long" is problematic with Windows 64bits. Long type on Windows 64bits is 32 bits. See at: https://docs.microsoft.com/pt-br/cpp/cpp/data-type-ranges?view=msvc-160 *long* 4 *long int*, *signed long int* -2.147.483.648 a 2.147.483.647 Therefore long never be > INT_MAX at Windows 64 bits. Thus lindex is always false in this expression: if (errno != 0 || badp == c || *badp != '\0' || lindex > INT_MAX || lindex < INT_MIN) Patch suggestion to fix this. diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 215a10f16e..54b0eded76 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -1675,7 +1675,7 @@ push_path(JsonbParseState **st, int level, Datum *path_elems, * end, the access index must be normalized by level. */ enum jbvType *tpath = palloc0((path_len - level) * sizeof(enum jbvType)); - long lindex; + int64 lindex; JsonbValue newkey; /* regards, Ranier Vilela jsonfuncs.patch Description: Binary data
Re: Operands don't affect result (CONSTANT_EXPRESSION_RESULT) (src/backend/utils/adt/jsonfuncs.c)
Em qui., 11 de fev. de 2021 às 01:46, Tom Lane escreveu: > Ranier Vilela writes: > > *long* 4 *long int*, *signed long int* -2.147.483.648 a 2.147.483.647 > > Therefore long never be > INT_MAX at Windows 64 bits. > > Thus lindex is always false in this expression: > > if (errno != 0 || badp == c || *badp != '\0' || lindex > INT_MAX || > lindex > > < INT_MIN) > > At the same time, I think this code could be improved; but the way > to do that is to use strtoint(), rather than kluging the choice of > datatype even further. > No matter the function used strtol or strtoint, push_path will remain broken with Windows 64bits. Or need to correct the expression. Definitely using long is a bad idea. regards, Ranier Vilela
Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)
Em qui., 11 de fev. de 2021 às 09:47, Michael Paquier escreveu: > On Wed, Feb 10, 2021 at 09:14:46AM -0300, Ranier Vilela wrote: > > It is necessary to correct the interfaces. To caller, inform the size of > > the buffer it created. > > Well, Coverity likes nannyism, so each one of its reports is to take > with a pinch of salt, so there is no point to change something that > does not make sense just to please a static analyzer. The context > of the code matters. > I do not agree. Coverity is a valuable tool that points to bad design functions. As demonstrated in the first email, it allows the user of the functions to corrupt memory. So it makes perfect sense, fixing the interface to prevent and prevent future modifications, simply breaking cryptohash api. > > Now, the patch you sent has no need to be that complicated, and it > partially works while not actually solving at all the problem you are > trying to solve (nothing done for MD5 or OpenSSL). Attached is an > example of what I finish with while poking at this issue. There is IMO > no point to touch the internals of SCRAM that all rely on the same > digest lengths for the proof generation with SHA256. > Too fast. I spent 30 minutes doing the patch. > > > I think it should be error-out, because the buffer can be malloc. > > I don't understand what you mean here, but cryptohash[_openssl].c > should not issue an error directly, just return a status code that the > caller can consume to generate an error. > I meant that it is not a case of assertion, as suggested by Kyotaro, because someone might want to create a dynamic buffer per malloc, to store the digest. Anyway, the buffer creator needs to tell the functions what the actual buffer size is, so they can decide what to do. regards, Ranier Vilela
Re: Operands don't affect result (CONSTANT_EXPRESSION_RESULT) (src/backend/utils/adt/jsonfuncs.c)
Em qui., 11 de fev. de 2021 às 14:51, Tom Lane escreveu: > Ranier Vilela writes: > > Em qui., 11 de fev. de 2021 às 01:46, Tom Lane > escreveu: > >> At the same time, I think this code could be improved; but the way > >> to do that is to use strtoint(), rather than kluging the choice of > >> datatype even further. > > > No matter the function used strtol or strtoint, push_path will remain > > broken with Windows 64bits. > > There is quite a lot of difference between "broken" and "my compiler > generates pointless warnings". Still, I changed it to use strtoint(), > because that's simpler and better style. > Thanks Tom, for fixing this. regards, Ranier Vilela
Re: pg_cryptohash_final possible out-of-bounds access (per Coverity)
Em qui., 11 de fev. de 2021 às 09:47, Michael Paquier escreveu: > On Wed, Feb 10, 2021 at 09:14:46AM -0300, Ranier Vilela wrote: > > It is necessary to correct the interfaces. To caller, inform the size of > > the buffer it created. > > Now, the patch you sent has no need to be that complicated, and it > partially works while not actually solving at all the problem you are > trying to solve (nothing done for MD5 or OpenSSL). Attached is an > example of what I finish with while poking at this issue. There is IMO > no point to touch the internals of SCRAM that all rely on the same > digest lengths for the proof generation with SHA256. > Ok, I take a look at your patch and I have comments: 1. Looks missed contrib/pgcrypto. 2. scram_HMAC_final function still have a exchanged parameters, which in the future may impair maintenance. Attached the v3 same patch. regards, Ranier Vilela pg_cryptohash_v3.patch Description: Binary data
Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
Hi, Per Coverity. It seems to me that some recent commit has failed to properly initialize a structure, in extended_stats.c, when is passed to heap_copytuple. regards, Ranier Vilela fix_uninitialized_var_extend_stats.patch Description: Binary data
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
Hi Justin, sorry for the delay.
Nothing against it, but I looked for similar codes and this is the usual
way to initialize HeapTupleData.
Perhaps InvalidOid makes a difference.
regards,
Ranier Vilela
Em dom., 11 de abr. de 2021 às 16:25, Justin Pryzby
escreveu:
> On Sun, Apr 11, 2021 at 03:38:10PM -0300, Ranier Vilela wrote:
> > Per Coverity.
> >
> > It seems to me that some recent commit has failed to properly initialize
> a
> > structure, in extended_stats.c, when is passed to heap_copytuple.
>
> I think you're right. You can look in the commit history to find the
> relevant
> commit and copy the committer.
>
> I think it's cleanest to write:
> |HeapTupleData tmptup = {0};
>
> --
> Justin
>
Re: Uninitialized scalar variable (UNINIT) (src/backend/statistics/extended_stats.c)
Em seg., 12 de abr. de 2021 às 03:04, Tom Lane escreveu:
> Michael Paquier writes:
> > On Sun, Apr 11, 2021 at 07:42:20PM -0300, Ranier Vilela wrote:
> >> Em dom., 11 de abr. de 2021 às 16:25, Justin Pryzby <
> pry...@telsasoft.com>
> >>> I think it's cleanest to write:
> >>> |HeapTupleData tmptup = {0};
>
> > I agree that this would be cleaner.
>
> It would be wrong, though, or at least not have the same effect.
>
I think that you speak about fill pointers with 0 is not the same as fill
pointers with NULL.
> ItemPointerSetInvalid does not set the target to all-zeroes.
>
ItemPointerSetInvalid set or not set the target to all-zeroes?
> (Regardless of that detail, it's generally best to accomplish
> objective X in the same way that existing code does. Deciding
> that you have a better way is often wrong, and even if you
> are right, you should then submit a patch to change all the
> existing cases.)
>
I was confused here, does the patch follow the pattern and fix the problem
or not?
regards,
Ranier Vilela
Re: Improving connection scalability: GetSnapshotData()
Latest Postgres Windows 64 bits msvc 2019 64 bits Patches applied v12-0001 to v12-0007: C:\dll\postgres\contrib\pgstattuple\pgstatapprox.c(74,28): warning C4013: 'GetOldestXmin' indefinido; assumindo extern retornando int [C:\dll\postgres C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(569,29): warning C4013: 'GetOldestXmin' indefinido; assumindo extern retornando int [C:\dll\postgres\pg_visibility. vcxproj] C:\dll\postgres\contrib\pgstattuple\pgstatapprox.c(74,56): error C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado [C:\dll\postgres\pgstattuple.vcxproj] C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(569,58): error C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado [C:\dll\postgres\pg_visibility.vcxproj] C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(686,70): error C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado [C:\dll\postgres\pg_visibility.vcxproj] regards, Ranier Vilela
Re: Improving connection scalability: GetSnapshotData()
Em sex., 24 de jul. de 2020 às 14:16, Andres Freund escreveu: > On 2020-07-24 14:05:04 -0300, Ranier Vilela wrote: > > Latest Postgres > > Windows 64 bits > > msvc 2019 64 bits > > > > Patches applied v12-0001 to v12-0007: > > > > C:\dll\postgres\contrib\pgstattuple\pgstatapprox.c(74,28): warning > C4013: > > 'GetOldestXmin' indefinido; assumindo extern retornando int > > [C:\dll\postgres > > C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(569,29): warning > > C4013: 'GetOldestXmin' indefinido; assumindo extern retornando int > > [C:\dll\postgres\pg_visibility. > > vcxproj] > > C:\dll\postgres\contrib\pgstattuple\pgstatapprox.c(74,56): error C2065: > > 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado > > [C:\dll\postgres\pgstattuple.vcxproj] > > C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(569,58): error > > C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado > > [C:\dll\postgres\pg_visibility.vcxproj] > > C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(686,70): error > > C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado > > [C:\dll\postgres\pg_visibility.vcxproj] > > I don't know that's about - there's no call to GetOldestXmin() in > pgstatapprox and pg_visibility after patch 0002? And similarly, the > PROCARRAY_* references are also removed in the same patch? > Maybe need to remove them from these places, not? C:\dll\postgres\contrib>grep -d GetOldestXmin *.c File pgstattuple\pgstatapprox.c: OldestXmin = GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM); File pg_visibility\pg_visibility.c: OldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM); * deadlocks, because surely GetOldestXmin() should never take RecomputedOldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM); regards, Ranier Vilela
Re: Improving connection scalability: GetSnapshotData()
Em sex., 24 de jul. de 2020 às 21:00, Andres Freund
escreveu:
> On 2020-07-24 18:15:15 -0300, Ranier Vilela wrote:
> > Em sex., 24 de jul. de 2020 às 14:16, Andres Freund
> > escreveu:
> >
> > > On 2020-07-24 14:05:04 -0300, Ranier Vilela wrote:
> > > > Latest Postgres
> > > > Windows 64 bits
> > > > msvc 2019 64 bits
> > > >
> > > > Patches applied v12-0001 to v12-0007:
> > > >
> > > > C:\dll\postgres\contrib\pgstattuple\pgstatapprox.c(74,28): warning
> > > C4013:
> > > > 'GetOldestXmin' indefinido; assumindo extern retornando int
> > > > [C:\dll\postgres
> > > > C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(569,29):
> warning
> > > > C4013: 'GetOldestXmin' indefinido; assumindo extern retornando int
> > > > [C:\dll\postgres\pg_visibility.
> > > > vcxproj]
> > > > C:\dll\postgres\contrib\pgstattuple\pgstatapprox.c(74,56): error
> C2065:
> > > > 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado
> > > > [C:\dll\postgres\pgstattuple.vcxproj]
> > > > C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(569,58):
> error
> > > > C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado
> > > > [C:\dll\postgres\pg_visibility.vcxproj]
> > > > C:\dll\postgres\contrib\pg_visibility\pg_visibility.c(686,70):
> error
> > > > C2065: 'PROCARRAY_FLAGS_VACUUM': identificador nao declarado
> > > > [C:\dll\postgres\pg_visibility.vcxproj]
> > >
> > > I don't know that's about - there's no call to GetOldestXmin() in
> > > pgstatapprox and pg_visibility after patch 0002? And similarly, the
> > > PROCARRAY_* references are also removed in the same patch?
> > >
> > Maybe need to remove them from these places, not?
> > C:\dll\postgres\contrib>grep -d GetOldestXmin *.c
> > File pgstattuple\pgstatapprox.c:
> > OldestXmin = GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM);
> > File pg_visibility\pg_visibility.c:
> > OldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM);
> > * deadlocks, because surely
> > GetOldestXmin() should never take
> > RecomputedOldestXmin =
> GetOldestXmin(NULL,
> > PROCARRAY_FLAGS_VACUUM);
>
> The 0002 patch changed those files:
>
> diff --git a/contrib/pg_visibility/pg_visibility.c
> b/contrib/pg_visibility/pg_visibility.c
> index 68d580ed1e0..37206c50a21 100644
> --- a/contrib/pg_visibility/pg_visibility.c
> +++ b/contrib/pg_visibility/pg_visibility.c
> @@ -563,17 +563,14 @@ collect_corrupt_items(Oid relid, bool all_visible,
> bool all_frozen)
> BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD);
> TransactionId OldestXmin = InvalidTransactionId;
>
> - if (all_visible)
> - {
> - /* Don't pass rel; that will fail in recovery. */
> - OldestXmin = GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM);
> - }
> -
> rel = relation_open(relid, AccessShareLock);
>
> /* Only some relkinds have a visibility map */
> check_relation_relkind(rel);
>
> + if (all_visible)
> + OldestXmin = GetOldestNonRemovableTransactionId(rel);
> +
> nblocks = RelationGetNumberOfBlocks(rel);
>
> /*
> @@ -679,11 +676,12 @@ collect_corrupt_items(Oid relid, bool all_visible,
> bool all_frozen)
> * From a concurrency point of view, it
> sort of sucks to
> * retake ProcArrayLock here while we're
> holding the buffer
> * exclusively locked, but it should be
> safe against
> -* deadlocks, because surely
> GetOldestXmin() should never take
> -* a buffer lock. And this shouldn't
> happen often, so it's
> -* worth being careful so as to avoid
> false positives.
> +* deadlocks, because surely
> GetOldestNonRemovableTransactionId()
> +* should never take a buffer lock. And
> this shouldn't happen
> +* often, so it's worth being careful so
> as to avoid false
> +* positives.
> */
> - RecomputedOldestXmin = GetOldestXmin(NULL,
> PROCARRAY_FLAGS_VACUUM);
> +
[ASAN] Postgres14 (Windows 64 bits)
Hi Tom, I'm starting tests using ASAN (address sanitizer) at Windows side, using msvc 2019 (built in asan support): First test report this: 2020-08-24 10:02:33.220 -03 postmaster[6656] LOG: starting PostgreSQL 14devel, compiled by Visual C++ build 1927, 64-bit 2020-08-24 10:02:33.228 -03 postmaster[6656] LOG: listening on IPv6 address "::1", port 58080 2020-08-24 10:02:33.228 -03 postmaster[6656] LOG: listening on IPv4 address "127.0.0.1", port 58080 2020-08-24 10:02:33.415 -03 startup[1604] LOG: database system was shut down at 2020-08-24 10:02:28 -03 2020-08-24 10:02:33.495 -03 postmaster[6656] LOG: database system is ready to accept connections 2020-08-24 10:02:34.580 -03 checkpointer[5680] LOG: checkpoint starting: immediate force wait flush-all 2020-08-24 10:02:34.598 -03 checkpointer[5680] LOG: checkpoint complete: wrote 3 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.006 s, sync=0.000 s, total=0.018 s; sync files=0, longest=0.000 s, average=0.000 s; distance=1 kB, estimate=1 kB 2020-08-24 10:02:35.146 -03 checkpointer[5680] LOG: checkpoint starting: immediate force wait 2020-08-24 10:02:35.155 -03 checkpointer[5680] LOG: checkpoint complete: wrote 0 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.002 s, sync=0.000 s, total=0.009 s; sync files=0, longest=0.000 s, average=0.000 s; distance=0 kB, estimate=1 kB = ==8400==AddressSanitizer CHECK failed: D:\agent\_work\9\s\src\vctools\crt\asan\llvm\compiler-rt\lib\asan\asan_thread.cc:356 "((ptr[0] == kCurrentStackFrameMagic)) != (0)" (0x0, 0x0) #0 0x7ffe985d0148 (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x180050148) #1 0x7ffe98597f3f (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x180017f3f) #2 0x7ffe985d5129 (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x180055129) #3 0x7ffe985b1de1 (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x180031de1) #4 0x7ffe985b0dea (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x180030dea) #5 0x7ffe985b30b5 (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x1800330b5) #6 0x7ffe985ce2bb (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x18004e2bb) #7 0x7ffe985d1d11 (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x180051d11) #8 0x14123da71 in dopr C:\dll\postgres\src\port\snprintf.c:441 #9 0x14123c127 in pg_vsnprintf C:\dll\postgres\src\port\snprintf.c:195 #10 0x141214cc0 in pvsnprintf C:\dll\postgres\src\common\psprintf.c:110 #11 0x14121cefe in appendStringInfoVA C:\dll\postgres\src\common\stringinfo.c:149 #12 0x14121cd9d in appendStringInfo C:\dll\postgres\src\common\stringinfo.c:103 #13 0x1411134c6 in send_message_to_server_log C:\dll\postgres\src\backend\utils\error\elog.c:2923 #14 0x14110d4f1 in EmitErrorReport C:\dll\postgres\src\backend\utils\error\elog.c:1456 #15 0x140c7537c in PostgresMain C:\dll\postgres\src\backend\tcop\postgres.c:4079 #16 0x140a98f28 in BackendRun C:\dll\postgres\src\backend\postmaster\postmaster.c:4530 #17 0x140a932ef in SubPostmasterMain C:\dll\postgres\src\backend\postmaster\postmaster.c:5053 #18 0x14069dfab in main C:\dll\postgres\src\backend\main\main.c:186 #19 0x1412694c8 in invoke_main D:\agent\_work\9\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78 #20 0x14126941d in __scrt_common_main_seh D:\agent\_work\9\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288 #21 0x1412692dd in __scrt_common_main D:\agent\_work\9\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:330 #22 0x141269538 in mainCRTStartup D:\agent\_work\9\s\src\vctools\crt\vcstartup\src\startup\exe_main.cpp:16 #23 0x7ffed1d46fd3 (C:\WINDOWS\System32\KERNEL32.DLL+0x180016fd3) #24 0x7ffed30fcec0 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x18004cec0) I'm not sure if ASAN can report false positives or if this CHECK error is own asan bug? Can you take a look, please? regards, Ranier Vilela
[PATCH] Resource leaks (src/backend/libpq/hba.c)
Hi Tom, Per Coverity. The function parse_hba_auth_op at (src/backend/libpq/hba.c) allows resource leaks when called by the function parse_hba_line, with parameters LOG and DEBUG3 levels. The function SplitGUCList (src/bin/pg_dump/dumputils.c) allows even returning FALSE, that namelist list is not empty and as memory allocated by pg_malloc. The simplest solution is free namelist, even when calling ereport, why the level can be LOG or DEBUG3. regards, Ranier Vilela PS. Are two SplitGUCList in codebase. 1. SplitGUCList (src/bin/pg_dump/dumputils.c) 2. SplitGUCList (src/backend/utils/adt/varlena.c) fix_resource_leak_dba.patch Description: Binary data
[PATCH] Dereference null return value (NULL_RETURNS) (src/backend/commands/statscmds.c)
Hi Tom, Per Coverity. The SearchSysCache1 allows return NULL and at function AlterStatistics, has one mistake, lack of, check of return, which enables a dereference NULL pointer, at function heap_modify_tuple. While there is room for improvement. Avoid calling SearchSysCache1 and table_open if the user "is not the owner of the existing statistics object". regards, Ranier Vilela fix_dereference_null_statscmds.patch Description: Binary data
[PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)
Hi Tom, Per Coverity. The variable root_offsets is read at line 1641, but, at this point, the content is unknown, so it is impossible to test works well. regards, Ranier Vilela fix_uninitialized_variable_heapam_handler.patch Description: Binary data
Out-of-bounds access (ARRAY_VS_SINGLETON) (src/backend/access/nbtree/nbtdedup.c)
Hi, Per Coverity. ARRAY vs SINGLETON If variable htids is accessed like array, but is a simple pointer, can be "This might corrupt or misinterpret adjacent memory locations." at line 723: /* Form standard non-pivot tuple */ itup->t_info &= ~INDEX_ALT_TID_MASK; htids = &itup->t_tid; 1. Here htids is a SINGLETON? So: At line 723: htids[ui++] = *BTreeTupleGetPostingN(origtuple, i); 2. htids is accessed how ARRAY? And is acessed at positions 0 and 1, according (nhtids == 1): Assert(ui == nhtids); The htids[1] are destroying something at this memory position. regards, Ranier Vilela
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)
Em ter., 25 de ago. de 2020 às 18:06, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Aug-25, Ranier Vilela wrote: > > > The variable root_offsets is read at line 1641, but, at this point, > > the content is unknown, so it is impossible to test works well. > > Surely it is set by heap_get_root_tuples() in line 1347? The root_blkno > variable is used exclusively to know whether root_offsets has been > initialized for the current block. > Hi Álvaro, 20. Condition hscan->rs_cblock != root_blkno, taking false branch. If the variable hscan->rs_cblock is InvalidBlockNumber the test can protect root_offsets fail. The var root_blkno only is checked at line 1853. regards, Ranier Vilela
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)
Em ter., 25 de ago. de 2020 às 19:45, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Aug-25, Ranier Vilela wrote: > > > If the variable hscan->rs_cblock is InvalidBlockNumber the test can > > protect root_offsets fail. > > When does that happen? > At first pass into the while loop? hscan->rs_cblock is InvalidBlockNumber, what happens? > > The var root_blkno only is checked at line 1853. > > That's a different function. > My mistake. Find editor. regards, Ranier Vilela
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)
Em ter., 25 de ago. de 2020 às 19:54, Ranier Vilela
escreveu:
> Em ter., 25 de ago. de 2020 às 19:45, Alvaro Herrera <
> alvhe...@2ndquadrant.com> escreveu:
>
>> On 2020-Aug-25, Ranier Vilela wrote:
>>
>> > If the variable hscan->rs_cblock is InvalidBlockNumber the test can
>> > protect root_offsets fail.
>>
>> When does that happen?
>>
> At first pass into the while loop?
> hscan->rs_cblock is InvalidBlockNumber, what happens?
>
> Other things.
1. Even heap_get_root_tuples at line 1347, be called.
Does it fill all roots_offsets?
root_offsets[offnum - 1] is secure at this point (line 1641 or is trash)?
2. hscan->rs_cbuf is constant?
if (hscan->rs_cblock != root_blkno)
{
Page page = BufferGetPage(hscan->rs_cbuf);
LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_SHARE);
heap_get_root_tuples(page, root_offsets);
LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_UNLOCK);
root_blkno = hscan->rs_cblock;
}
This can move outside while loop?
Am I wrong or hscan do not change?
regards,
Ranier Vilela
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)
Em ter., 25 de ago. de 2020 às 20:13, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Aug-25, Ranier Vilela wrote: > > > Em ter., 25 de ago. de 2020 às 19:45, Alvaro Herrera < > > alvhe...@2ndquadrant.com> escreveu: > > > > > On 2020-Aug-25, Ranier Vilela wrote: > > > > > > > If the variable hscan->rs_cblock is InvalidBlockNumber the test can > > > > protect root_offsets fail. > > > > > > When does that happen? > > > > At first pass into the while loop? > > hscan->rs_cblock is InvalidBlockNumber, what happens? > > No, it is set when the page is read. > And it is guaranteed that, rs_cblock is not InvalidBlockNumber when the page is read? Ranier Vilela
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)
Em ter., 25 de ago. de 2020 às 20:20, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Aug-25, Ranier Vilela wrote: > > > And it is guaranteed that, rs_cblock is not InvalidBlockNumber when the > > page is read? > > It could be InvalidBlockNumber if sufficient neutrinos hit the memory > bank and happen to set all the bits in the block number. > kkk, I think it's enough for me. I believe that PostgreSQL will not run on the ISS yet. Ranier Vilela
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)
Em ter., 25 de ago. de 2020 às 20:29, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Aug-25, Ranier Vilela wrote: > > > kkk, I think it's enough for me. > > I believe that PostgreSQL will not run on the ISS yet. > > Actually, I believe there are some satellites that run Postgres -- not > 100% sure about this. > Yeah, ESA uses: https://resources.2ndquadrant.com/european-space-agency-case-study-download In fact, Postgres is to be congratulated. Guess who didn't make any bug? https://changochen.github.io/publication/squirrel_ccs2020.pdf "Sqirrel has successfully detected 63 bugs from tested DBMS,including 51 bugs from SQLite, 7 from MySQL, and 5 from MariaDB." Ranier Vilela
Re: [PATCH] Resource leaks (src/backend/libpq/hba.c)
Em ter., 25 de ago. de 2020 às 23:02, Kyotaro Horiguchi < horikyota@gmail.com> escreveu: > At Tue, 25 Aug 2020 10:20:07 -0300, Ranier Vilela > wrote in > > Hi Tom, > > > > Per Coverity. > > > > The function parse_hba_auth_op at (src/backend/libpq/hba.c) allows > resource > > leaks when called > > by the function parse_hba_line, with parameters LOG and DEBUG3 levels. > > The function SplitGUCList (src/bin/pg_dump/dumputils.c) allows even > > returning FALSE, > > that namelist list is not empty and as memory allocated by pg_malloc. > > As you know, there are two implementations of the function. One that > uses pg_malloc is used in pg_dump and the returned char *namelist is > always pg_free'd after use. The other that returns a pg_list, and the > returned list is reclaimed by MemoryContextDelete at callers > (concretely load_hba and fill_hba_view). Indeed they share the same > name but have different signatures so the two are statically > distinguishable but Coverity seems failing to do so. You may need to > avoid feeding the whole source tree to Coverity at once. > Yes, thanks for the hit. > > Anyway this is a very common style in the PostgreSQL code so I > recommend to verify the outcome from such tools against the actual > code. > Ok. > > The simplest solution is free namelist, even when calling ereport, why > the > > level can be > > LOG or DEBUG3. > > So we don't need to do anything there. Rather we can remove the > existing list_free(parsed_servers) in parse_hba_auth_opt. > It would be good, the call helped to confuse. Very thanks, for the explanation. Ranier Vilela
Clang Address Sanitizer (Postgres14) Detected Memory Leaks
Hi, Is this something to worry about, or is it another problem with the analysis tool, that nobody cares about? clang 10 (64 bits) postgres 14 (latest) 31422==ERROR: LeakSanitizer: detected memory leaks Direct leak of 4560 byte(s) in 1 object(s) allocated from: #0 0x50e33d in malloc (/usr/src/postgres/tmp_install/usr/local/pgsql/bin/postgres+0x50e33d) #1 0x186d52f in ConvertTimeZoneAbbrevs /usr/src/postgres/src/backend/utils/adt/datetime.c:4511:8 #2 0x1d9b5e9 in load_tzoffsets /usr/src/postgres/src/backend/utils/misc/tzparser.c:465:12 #3 0x1d8ca3f in check_timezone_abbreviations /usr/src/postgres/src/backend/utils/misc/guc.c:11389:11 #4 0x1d6a398 in call_string_check_hook /usr/src/postgres/src/backend/utils/misc/guc.c:11056:7 #5 0x1d68f29 in parse_and_validate_value /usr/src/postgres/src/backend/utils/misc/guc.c:6870:10 #6 0x1d6567d in set_config_option /usr/src/postgres/src/backend/utils/misc/guc.c:7473:11 #7 0x1d7f8f4 in ProcessGUCArray /usr/src/postgres/src/backend/utils/misc/guc.c:10608:10 #8 0x9d0c8d in ApplySetting /usr/src/postgres/src/backend/catalog/pg_db_role_setting.c:256:4 #9 0x1d4ad93 in process_settings /usr/src/postgres/src/backend/utils/init/postinit.c:1174:2 #10 0x1d48e39 in InitPostgres /usr/src/postgres/src/backend/utils/init/postinit.c:1059:2 #11 0x14a2c1a in BackgroundWorkerInitializeConnectionByOid /usr/src/postgres/src/backend/postmaster/postmaster.c:5758:2 #12 0x853feb in ParallelWorkerMain /usr/src/postgres/src/backend/access/transam/parallel.c:1373:2 #13 0x146e5fb in StartBackgroundWorker /usr/src/postgres/src/backend/postmaster/bgworker.c:813:2 #14 0x14af69b in do_start_bgworker /usr/src/postgres/src/backend/postmaster/postmaster.c:5879:4 #15 0x14a1487 in maybe_start_bgworkers /usr/src/postgres/src/backend/postmaster/postmaster.c:6104:9 #16 0x149e5aa in sigusr1_handler /usr/src/postgres/src/backend/postmaster/postmaster.c:5269:3 #17 0x7fcffa75a3bf (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf) #18 0x149d655 in PostmasterMain /usr/src/postgres/src/backend/postmaster/postmaster.c:1414:11 #19 0x108402e in main /usr/src/postgres/src/backend/main/main.c:209:3 #20 0x7fcffa54e0b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16 Direct leak of 1020 byte(s) in 15 object(s) allocated from: #0 0x4fa6e4 in strdup (/usr/src/postgres/tmp_install/usr/local/pgsql/bin/postgres+0x4fa6e4) #1 0x1d6a1c7 in guc_strdup /usr/src/postgres/src/backend/utils/misc/guc.c:4889:9 #2 0x1d7efc7 in set_config_sourcefile /usr/src/postgres/src/backend/utils/misc/guc.c:7696:15 #3 0x1d7c95e in ProcessConfigFileInternal /usr/src/postgres/src/backend/utils/misc/guc-file.l:478:4 #4 0x1d5b33f in ProcessConfigFile /usr/src/postgres/src/backend/utils/misc/guc-file.l:156:9 #5 0x1d5ae7d in SelectConfigFiles /usr/src/postgres/src/backend/utils/misc/guc.c:5674:2 #6 0x149b6ce in PostmasterMain /usr/src/postgres/src/backend/postmaster/postmaster.c:884:7 Ranier Vilela
Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Hi, Per Clang UBSan Clang 10 (64 bits) Postgres 14 (latest) 2020-08-27 01:02:14.930 -03 client backend[42432] pg_regress/create_table STATEMENT: create table defcheck_0 partition of defcheck for values in (0); indexcmds.c:1162:22: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:44:28: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior indexcmds.c:1162:22 in clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:65:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior clog.c:299:10 in indexcmds.c (1162): memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts); clog.c (299): memcmp(subxids, MyProc->subxids.xids, nsubxids * sizeof(TransactionId)) == 0) xact.c (5285) memcpy(&workspace[i], s->childXids, s->nChildXids * sizeof(TransactionId)); snapmgr.c (590) memcpy(CurrentSnapshot->xip, sourcesnap->xip, sourcesnap->xcnt * sizeof(TransactionId)); snapmgr.c (594) memcpy(CurrentSnapshot->subxip, sourcesnap->subxip, sourcesnap->subxcnt * sizeof(TransactionId)); copyfuncs.c:1190 COPY_POINTER_FIELD(uniqColIdx, from->uniqNumCols * sizeof(AttrNumber)); 1.STATEMENT: CREATE TABLESPACE regress_tblspacewith LOCATION '/usr/src/postgres/src/test/regress/testtablespace' WITH (some_nonexistent_parameter = true); clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null 2.STATEMENT: CREATE TABLE testschema.dflt (a int PRIMARY KEY USING INDEX TABLESPACE regress_tblspace) PARTITION BY LIST (a); indexcmds.c:1162:22: runtime error: null pointer passed as argument 2, which is declared to never be null 3.STATEMENT: SELECT bool 'nay' AS error; clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null 4.STATEMENT: SELECT U&'wrong: +0061' UESCAPE '+'; clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null 5. STATEMENT: ALTER TABLE circles ADD EXCLUDE USING gist (c1 WITH &&, (c2::circle) WITH &&); xact.c:5285:25: runtime error: null pointer passed as argument 2, which is declared to never be null 6.STATEMENT: COMMENT ON CONSTRAINT the_constraint ON DOMAIN no_comments_dom IS 'another bad comment'; snapmgr.c:590:31: runtime error: null pointer passed as argument 2, which is declared to never be null 7.STATEMENT: create trigger my_table_col_update_trig after update of b on my_table referencing new table as new_table for each statement execute procedure dump_insert(); clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:65:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior clog.c:299:10 in xact.c:5285:25: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:44:28: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior xact.c:5285:25 in snapmgr.c:590:31: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:44:28: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior snapmgr.c:590:31 in snapmgr.c:594:34: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:44:28: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior snapmgr.c:594:34 in clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:65:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior clog.c:299:10 in clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:65:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior clog.c:299:10 in 8. clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:65:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior clog.c:299:10 in clog.c:299:10: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:65:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior clog.c:299:10 in 8.STATEMENT: select array_fill(1, array[[1,2],[3,4]]); copyfuncs.c:1190:2: runtime error: null pointer passed as argument 2, which is declared to never be null I stopped counting clog.c (299). If anyone wants, the full report, it has 2mb. Ranier Vilela
re: Please help for error ( file is required for XML support )
>checking for libxml/parser.h... no >configure: error: header file is required for XML support sudo yum install libxml2-devel You need to install the development package before ./configure. regards, Ranier Vilela
Re: Clang Address Sanitizer (Postgres14) Detected Memory Leaks
Em qui., 27 de ago. de 2020 às 12:46, Tom Lane escreveu: > Ranier Vilela writes: > > Is this something to worry about, or is it another problem with the > > analysis tool, that nobody cares about? > > As far as the first one goes, I'd bet on buggy analysis tool. > The complained-of allocation is evidently for the "extra" state > associated with the timezone GUC variable, and AFAICS guc.c is > quite careful not to leak those. It is true that the block will > still be allocated at process exit, but that doesn't make it a leak. > > I did not trace the second one in any detail, but I don't believe > guc.c leaks sourcefile strings either. There's only one place > where it overwrites them, and that place frees the old value. > > If these allocations do genuinely get leaked in some code path, > this report is of exactly zero help in finding where; and I'm > afraid I'm not very motivated to go looking for a bug that probably > doesn't exist. > Hi Tom, thanks for taking a look at this. I tried to find where the zone table is freed, without success. It would be a big surprise for me, if this tool is buggy. Anyway, it's just a sample of the total report, which is 10 mb (postmaster.log), done with the regression tests. regards, Ranier Vilela
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Em qui., 27 de ago. de 2020 às 13:57, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Aug-27, Ranier Vilela wrote: > > > indexcmds.c (1162): > > memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts); > > Looks legit, and at least per commit 13bba02271dc we do fix such things, > even if it's useless in practice. > > Given that no buildfarm member has ever complained, this exercise seems > pretty pointless. > Hi Álvaro, If we are passing a null pointer in these places and it should not be done, it is a sign that perhaps these calls should not or should not be made, and they can be avoided. This would eliminate undefined behavior and save some cycles? regards, Ranier Vilela
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Em qui., 27 de ago. de 2020 às 13:57, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Aug-27, Ranier Vilela wrote: > > > indexcmds.c (1162): > > memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts); > > Looks legit, and at least per commit 13bba02271dc we do fix such things, > even if it's useless in practice. > > Given that no buildfarm member has ever complained, this exercise seems > pretty pointless. > See at: https://postgrespro.com/list/thread-id/1870065 "NULL passed as an argument to memcmp() in parse_func.c <https://postgrespro.com/list/id/blu437-smtp48a5b2099e7134ac6be7c7f2...@phx.gbl> " regards, Ranier Vilela
Re: Clang Address Sanitizer (Postgres14) Detected Memory Leaks
More reports. Memory Sanitizer: running bootstrap script ... ==40179==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x538cfc1 in pg_comp_crc32c_sb8 /usr/src/postgres/src/port/pg_crc32c_sb8.c:80:4 #1 0x533a0c0 in pg_comp_crc32c_choose /usr/src/postgres/src/port/pg_crc32c_sse42_choose.c:61:9 #2 0xebbdae in BootStrapXLOG /usr/src/postgres/src/backend/access/transam/xlog.c:5293:2 #3 0xfc5867 in AuxiliaryProcessMain /usr/src/postgres/src/backend/bootstrap/bootstrap.c:437:4 #4 0x26a12c3 in main /usr/src/postgres/src/backend/main/main.c:201:3 #5 0x7f035d0e90b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16 #6 0x495afd in _start (/usr/src/postgres/tmp_install/usr/local/pgsql/bin/postgres+0x495afd) Uninitialized value was stored to memory at #0 0x538cbaa in pg_comp_crc32c_sb8 /usr/src/postgres/src/port/pg_crc32c_sb8.c:72:15 #1 0x533a0c0 in pg_comp_crc32c_choose /usr/src/postgres/src/port/pg_crc32c_sse42_choose.c:61:9 #2 0xebbdae in BootStrapXLOG /usr/src/postgres/src/backend/access/transam/xlog.c:5293:2 #3 0xfc5867 in AuxiliaryProcessMain /usr/src/postgres/src/backend/bootstrap/bootstrap.c:437:4 #4 0x26a12c3 in main /usr/src/postgres/src/backend/main/main.c:201:3 #5 0x7f035d0e90b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16 Uninitialized value was stored to memory at #0 0x538c836 in pg_comp_crc32c_sb8 /usr/src/postgres/src/port/pg_crc32c_sb8.c:57:11 #1 0x533a0c0 in pg_comp_crc32c_choose /usr/src/postgres/src/port/pg_crc32c_sse42_choose.c:61:9 #2 0xebbdae in BootStrapXLOG /usr/src/postgres/src/backend/access/transam/xlog.c:5293:2 #3 0xfc5867 in AuxiliaryProcessMain /usr/src/postgres/src/backend/bootstrap/bootstrap.c:437:4 #4 0x26a12c3 in main /usr/src/postgres/src/backend/main/main.c:201:3 #5 0x7f035d0e90b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16 Uninitialized value was stored to memory at #0 0x49b666 in __msan_memcpy (/usr/src/postgres/tmp_install/usr/local/pgsql/bin/postgres+0x49b666) #1 0xebbb70 in BootStrapXLOG /usr/src/postgres/src/backend/access/transam/xlog.c:5288:2 #2 0xfc5867 in AuxiliaryProcessMain /usr/src/postgres/src/backend/bootstrap/bootstrap.c:437:4 #3 0x26a12c3 in main /usr/src/postgres/src/backend/main/main.c:201:3 #4 0x7f035d0e90b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16 Uninitialized value was created by an allocation of 'checkPoint' in the stack frame of function 'BootStrapXLOG' #0 0xeb9f50 in BootStrapXLOG /usr/src/postgres/src/backend/access/transam/xlog.c:5194 This line solve the alert: (xlog.c) 5193: memset(&checkPoint, 0, sizeof(checkPoint)); I'm starting to doubt this tool. regards, Ranier Vilela
[PATCH] Explicit null dereferenced (src/backend/access/heap/heaptoast.c)
Hi, Per Coverity. When "Prepare for toasting", it is necessary to turn off the flag TOAST_NEEDS_DELETE_OLD, if there is no need to delete external values from the old tuple, otherwise, there are dereference NULL at toast_helper.c (on toast_tuple_cleanup function). regards, Ranier Vilela fix_null_dereference_heaptoast.patch Description: Binary data
Re: [PATCH] Explicit null dereferenced (src/backend/access/heap/heaptoast.c)
Em sex., 28 de ago. de 2020 às 04:45, escreveu:
>
>
>
>
>
> ‐‐‐ Original Message ‐‐‐
> On Friday, 28 August 2020 03:22, Ranier Vilela
> wrote:
>
> > Hi,
> >
> > Per Coverity.
> >
> > When "Prepare for toasting", it is necessary to turn off the flag
> TOAST_NEEDS_DELETE_OLD,
> > if there is no need to delete external values from the old tuple,
> otherwise,
> > there are dereference NULL at toast_helper.c (on toast_tuple_cleanup
> function).
> >
>
> Excuse my ignorance, isn't this a false positive?
>
Yes, you're right.
Coverity fails with &.
if (oldtup == NULL)
147{
3. assign_zero: Assigning: ttc.ttc_oldvalues = NULL.
148ttc.ttc_oldvalues = NULL;
149ttc.ttc_oldisnull = NULL;
4. Falling through to end of if statement.
150}
151else
152{
153ttc.ttc_oldvalues = toast_oldvalues;
154ttc.ttc_oldisnull = toast_oldisnull;
155}
156ttc.ttc_attr = toast_attr;
157toast_tuple_init(&ttc); // Coverity ignores the call completely
here.
toast_tuple_init, solves the bug, because reset ttc->flags.
> Regardless right after prepare for toasting, a call to toast_tuple_init is
> made which will explicitly and unconditionally set ttc_flags to zero so the
> flag bit set in the patch will be erased anyways. This patch may make
> coverity happy but does not really change anything in the behaviour of the
> code.
>
That's right, the patch doesn't change anything.
>
> Furthermore, in the same function, toast_tuple_init, the flag is set to
> TOAST_NEEDS_DELETE_OLD after the old value is actually inspected and found
> to not be null, be stored on disk and to be different than the new value.
> To my understanding, this seems to be correct.
>
Very correct.
Thanks for taking a look here.
You could take a look at the attached patch,
would it be an improvement?
toast_tuple_init, it seems to me that it can be improved.
ttc->ttc_oldvalues is constant, and it could be unlooping?
regards,
Ranier Vilela
unloop_toast_tuple_init.patch
Description: Binary data
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Em sex., 28 de ago. de 2020 às 00:11, Tom Lane escreveu: > > In other words, the C standard made a damfool decision and now we need > to deal with the consequences of that as perpetrated by other fools. > Still, it's all hypothetical so far --- does anyone have examples of > actual rather than theoretical issues? > I still think the value of this alert would be to avoid the call. Why do memcmp have to deal with a NULL value? clog.c: 299, it is a case outside the curve, there are hundreds of calls in the report. It must be very difficult to correct, but if TransactionIdSetPageStatus was not called in these cases, memcmp, it would not have to deal with the NULL pointer. regards, Ranier Vilela
Re: Please help for error ( file is required for XML support )
Em sex., 28 de ago. de 2020 às 04:37, Sachin Khanna < sachin_kha...@infosys.com> escreveu: > > > Now I can see the that libraries are identified in config.log . I also > found some error in configuration file as well. Highlight below. > > Please find the attached config.log file for reference. > > Please help and let me know if I there is any issue in configuration > script and I need to change it ? > > > > I am using PostgreSQL version postgresql-12.3.tar.gz > > > > configure:8291: checking for XML2_CONFIG > > configure:8293: result: /usr/lib64/libxml2.so > > configure:8416: checking for grep that handles long lines and -e > > configure:8474: result: /bin/grep > > configure:8479: checking for egrep > > configure:8541: result: /bin/grep -E > > configure:8588: checking for ld used by GCC > > configure:8651: result: /bin/ld > > configure:8658: checking if the linker (/bin/ld) is GNU ld > > GNU ld version 2.27-44.base.el7 > > > > at-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -O2 > -D_GNU_SOURCE conftest.c >&5 > > conftest.c: In function 'main': > > *conftest.c:76:20: error: expected expression before ')' token* > > *if (sizeof ((_Bool)))* > > * ^* > > *configure:12910: $? = 1* > > *configure: failed program was:* > > *| /* confdefs.h */* > What version of gcc is being used in this environment? regards, Ranier Vilela
Re: Please help for error ( file is required for XML support )
em sex., 28 de ago. de 2020 às 11:05, Ranier Vilela escreveu: > Em sex., 28 de ago. de 2020 às 04:37, Sachin Khanna < > sachin_kha...@infosys.com> escreveu: > >> >> >> Now I can see the that libraries are identified in config.log . I also >> found some error in configuration file as well. Highlight below. >> >> Please find the attached config.log file for reference. >> >> Please help and let me know if I there is any issue in configuration >> script and I need to change it ? >> >> >> >> I am using PostgreSQL version postgresql-12.3.tar.gz >> >> >> >> configure:8291: checking for XML2_CONFIG >> >> configure:8293: result: /usr/lib64/libxml2.so >> >> configure:8416: checking for grep that handles long lines and -e >> >> configure:8474: result: /bin/grep >> >> configure:8479: checking for egrep >> >> configure:8541: result: /bin/grep -E >> >> configure:8588: checking for ld used by GCC >> >> configure:8651: result: /bin/ld >> >> configure:8658: checking if the linker (/bin/ld) is GNU ld >> >> GNU ld version 2.27-44.base.el7 >> >> >> >> at-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -O2 >> -D_GNU_SOURCE conftest.c >&5 >> >> conftest.c: In function 'main': >> >> *conftest.c:76:20: error: expected expression before ')' token* >> >> *if (sizeof ((_Bool)))* >> >> *^* >> >> *configure:12910: $? = 1* >> >> *configure: failed program was:* >> >> *| /* confdefs.h */* >> > What version of gcc is being used in this environment? > It seems gcc does not compile C99, without -std=c99 configure:4421: checking for gcc option to accept ISO C99 conftest.c:130:3: note: use option -std=c99 or -std=gnu99 to compile your code try add -std=c99 ./configure -std=c99 regards, Ranier Vilela
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Em sex., 28 de ago. de 2020 às 03:04, Noah Misch escreveu: > On Thu, Aug 27, 2020 at 11:11:47PM -0400, Tom Lane wrote: > > Noah Misch writes: > > > On Thu, Aug 27, 2020 at 12:57:20PM -0400, Alvaro Herrera wrote: > > >> Looks legit, and at least per commit 13bba02271dc we do fix such > things, > > >> even if it's useless in practice. > > >> Given that no buildfarm member has ever complained, this exercise > seems > > >> pretty pointless. > > > > > Later decision to stop changing such code: > > > > https://postgr.es/m/flat/e1a26ece-7057-a234-d87e-4ce1cdc9e...@2ndquadrant.com > > > I think that the actual risk here has to do not with > > what memcmp() might do, but with what gcc might do in code surrounding > > the call, once it's armed with the assumption that any pointer we pass > > to memcmp() could not be null. See > > > > > https://www.postgresql.org/message-id/flat/BLU437-SMTP48A5B2099E7134AC6BE7C7F2A10%40phx.gbl > > > > It's surely not hard to visualize cases where necessary code could > > be optimized away if the compiler thinks it's entitled to assume > > such things. > > Good point. We could pick from a few levels of concern: > > - No concern: reject changes serving only to remove this class of > deviation. > This is today's policy. > - Medium concern: accept fixes, but the buildfarm continues not to break in > the face of new deviations. This will make some code uglier, but we'll > be > ready against some compiler growing the optimization you describe. > - High concern: I remove -fno-sanitize=nonnull-attribute from buildfarm > member > thorntail. In addition to the drawback of the previous level, this will > create urgent work for committers introducing new deviations (or > introducing > test coverage that unearths old deviations). This is our current > response > to Valgrind complaints, for example. > Maybe in this specific case, the policy could be ignored, this change does not hurt. --- a/src/backend/access/transam/clog.c +++ b/src/backend/access/transam/clog.c @@ -293,7 +293,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 (all_xact_same_page && subxids && xid == MyProc->xid && nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT && nsubxids == MyProc->subxidStatus.count && memcmp(subxids, MyProc->subxids.xids, regards, Ranier Vilela
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
More troubles with undefined-behavior.
This type of code can leaves overflow:
var = (cast) (expression);
diff = (int32) (id1 - id2);
See:
diff64 = ((long int) d1 - (long int) d2);
diff64=-4294901760
#include
#include
int main()
{
unsigned int d1 = 3;
unsigned int d2 = 4294901763;
unsigned int diffu32 = 0;
unsigned long int diffu64 = 0;
unsigned long int diff64 = 0;
int32_t diff = 0;
diff = (int32_t) (d1 - d2);
diff64 = ((long int) d1 - (long int) d2);
diffu32 = (unsigned int) (d1 - d2);
diffu64 = (unsigned long int) (d1 - d2);
printf("d1=%u\n", d1);
printf("d2=%u\n", d2);
printf("diff=%d\n", diff);
printf("diffu32=%u\n", diffu32);
printf("diff64=%ld\n", diff64);
printf("diffu64=%lu\n", diffu64);
return 0;
}
output:
d1=3
d2=4294901763
diff=65536
diffu32=65536
diff64=-4294901760
diffu64=65536
(With Ubuntu 64 bits + clang 10)
transam.c:311:22: runtime error: unsigned integer overflow: 3 - 4294901763
cannot be represented in type 'unsigned int'
TransactionIdPrecedes(TransactionId id1, TransactionId id2)
{
/*
* If either ID is a permanent XID then we can just do unsigned
* comparison. If both are normal, do a modulo-2^32 comparison.
*/
int32 diff;
if (!TransactionIdIsNormal(id1) || !TransactionIdIsNormal(id2))
return (id1 < id2);
diff = (int32) (id1 - id2);
return (diff < 0);
}
This works, all time or really with bad numbers can break?
I would like to know, why doesn't it work?
With Windows 10 (64 bits) + msvc 2019 (64 bits)
bool
TransactionIdPrecedes(TransactionId id1, TransactionId id2)
{
/*
* If either ID is a permanent XID then we can just do unsigned
* comparison. If both are normal, do a modulo-2^32 comparison.
*/
int32 diff;
int64 diff64;
if (!TransactionIdIsNormal(id1) || !TransactionIdIsNormal(id2))
return (id1 < id2);
diff = (int32) (id1 - id2);
diff64 = ((int64) id1 - (int64) id2);
fprintf(stderr, "id1=%lu\n", id1);
fprintf(stderr, "id2=%lu\n", id1);
fprintf(stderr, "diff32=%ld\n", diff);
fprintf(stderr, "diff64=%lld\n", diff64);
return (diff64 < 0);
}
id1=498
id2=498
diff32=2
diff64=-4094967296
2020-08-31 12:46:30.422 -03 [8908] WARNING: oldest xmin is far in the past
2020-08-31 12:46:30.422 -03 [8908] HINT: Close open transactions soon to
avoid wraparound problems.
You might also need to commit or roll back old prepared transactions, or
drop stale replication slots.
id1=4
id2=4
diff32=-494
diff64=-494
id1=4
id2=4
diff32=5000
diff64=-4244967296
2020-08-31 12:46:30.423 -03 [8908] FATAL: found xmin 4 from before
relfrozenxid 4244967300
2020-08-31 12:46:30.423 -03 [8908] CONTEXT: while scanning block 0 and
offset 1 of relation "pg_catalog.pg_depend"
2020-08-31 12:46:30.423 -03 [8908] STATEMENT: VACUUM FREEZE;
Most of the time:
id1=498
id2=498
diff32=0
diff64=0
id1=498
id2=498
diff32=0
diff64=0
regards,
Ranier Vilela
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Em seg., 31 de ago. de 2020 às 14:00, Alvaro Herrera <
alvhe...@2ndquadrant.com> escreveu:
> On 2020-Aug-31, Ranier Vilela wrote:
>
> > More troubles with undefined-behavior.
> >
> > This type of code can leaves overflow:
> > var = (cast) (expression);
> > diff = (int32) (id1 - id2);
> >
> > See:
> > diff64 = ((long int) d1 - (long int) d2);
> > diff64=-4294901760
>
> Did you compile this with gcc -fwrapv?
>
gcc 10.2 -O2 -fwrapv
bool test1()
{
unsigned int d1 = 3;
unsigned int d2 = 4294901763;
long int diff64 = 0;
diff64 = ((long int) d1 - (long int) d2);
return (diff64 < 0);
}
output:
mov eax, 1
ret
What is a workaround for msvc 2019 (64 bits) and clang 64 bits (linux)?
transam.c:311:22: runtime error: unsigned integer overflow: 3 - 4294901763
cannot be represented in type 'unsigned int'
Ranier Vilela
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Em seg., 31 de ago. de 2020 às 14:43, Ranier Vilela
escreveu:
> Em seg., 31 de ago. de 2020 às 14:00, Alvaro Herrera <
> alvhe...@2ndquadrant.com> escreveu:
>
>> On 2020-Aug-31, Ranier Vilela wrote:
>>
>> > More troubles with undefined-behavior.
>> >
>> > This type of code can leaves overflow:
>> > var = (cast) (expression);
>> > diff = (int32) (id1 - id2);
>> >
>> > See:
>> > diff64 = ((long int) d1 - (long int) d2);
>> > diff64=-4294901760
>>
>> Did you compile this with gcc -fwrapv?
>>
> gcc 10.2 -O2 -fwrapv
> bool test1()
> {
> unsigned int d1 = 3;
> unsigned int d2 = 4294901763;
> long int diff64 = 0;
>
> diff64 = ((long int) d1 - (long int) d2);
>
> return (diff64 < 0);
> }
>
> output:
> mov eax, 1
> ret
>
> What is a workaround for msvc 2019 (64 bits) and clang 64 bits (linux)?
> transam.c:311:22: runtime error: unsigned integer overflow: 3 - 4294901763
> cannot be represented in type 'unsigned int'
>
with Debug:
#include
#include
bool test1(void)
{
unsigned int d1 = 3;
unsigned int d2 = 4294901763;
int32_t diff;
diff = (int32_t) (d1 - d2);
return (diff < 0);
}
gcc 10.2 -g
output:
pushrbp
mov rbp, rsp
mov DWORD PTR [rbp-4], 3
mov DWORD PTR [rbp-8], -65533
mov eax, DWORD PTR [rbp-4]
sub eax, DWORD PTR [rbp-8]
mov DWORD PTR [rbp-12], eax
mov eax, DWORD PTR [rbp-12]
shr eax, 31
pop rbp
ret
it is possible to conclude that:
1. TransactionIdPrecedes works in release mode, because the compiler treats
undefined-behavior and corrects it,
treating a possible overflow.
2. TransactionIdPrecedes does not work in debug mode, and overflow occurs.
3. TransactionID cannot contain the largest possible ID or an invalid ID
(4294901763) has been generated and passed to TransactionIdPrecedes.
Ranier Vilela
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Em seg., 31 de ago. de 2020 às 16:39, Peter Geoghegan escreveu: > On Mon, Aug 31, 2020 at 11:42 AM Andres Freund wrote: > > Unsigned integer overflow is well defined in the standard. So I don't > understand what this is purporting to warn about. > > Presumably it's simply warning that the value -4294901760 (i.e. the > result of 3 - 4294901763) cannot be faithfully represented as an > unsigned int. This is true, of course. It's just not relevant. > > I'm pretty sure that UBSan does not actually state that this is > undefined behavior. At least Ranier's sample output didn't seem to > indicate it. > 4294901763 can not store at unsigned int (TransactionID is uint32_t). TransactionId id2 at TransactionIdPrecedes already has an overflow, before anything is done. Ranier Vilela
Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior
Em seg., 31 de ago. de 2020 às 17:05, Andres Freund escreveu: > Hi, > > On 2020-08-31 12:38:51 -0700, Peter Geoghegan wrote: > > On Mon, Aug 31, 2020 at 11:42 AM Andres Freund > wrote: > > > Unsigned integer overflow is well defined in the standard. So I don't > understand what this is purporting to warn about. > > > > Presumably it's simply warning that the value -4294901760 (i.e. the > > result of 3 - 4294901763) cannot be faithfully represented as an > > unsigned int. This is true, of course. It's just not relevant. > > > > I'm pretty sure that UBSan does not actually state that this is > > undefined behavior. At least Ranier's sample output didn't seem to > > indicate it. > > Well, my point is that there's no point in discussing unsigned integer > overflow, since it's precisely specified. And hence I don't understand > what we're discussing in this sub-thread. > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html says: > > > -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where > > the result of an unsigned integer computation cannot be represented in > > its type. Unlike signed integer overflow, this is not undefined > > behavior, but it is often unintentional. This sanitizer does not check > > for lossy implicit conversions performed before such a computation > > (see -fsanitize=implicit-conversion). > > So it seems Rainier needs to turn this test off, because it actually is > intentional. > No problem. If intentional, the code at TransactionIdPrecedes, already knows that overflow can occur and trusts that the compiler will save it. Ranier Vilela
[NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
Hi, Is possible that BTreeTupleSetNAtts, leave everything tidy, so that BTreeTupleGetHeapTID doesn't fail. BTreeTupleGetHeapTID can return NULL. But, as we can see: 1. Line 2085 (nbtutils.c): if (BTreeTupleGetHeapTID(itup) != NULL && tupnatts != nkeyatts) 2. Line 803 (nbtsearch.c): if (heapTid == NULL) Maybe, better make sure, because: 3. Line 2285 (nbtutils.c): ItemPointerCopy(BTreeTupleGetMaxHeapTID(lastleft), pivotheaptid); 4. Line 2316 (nbtutils.c) : ItemPointerCopy(BTreeTupleGetHeapTID(firstright), pivotheaptid); Can dereference NULL pointer (pivotheaptid) at runtime (release version). itemptr.h: #define ItemPointerCopy(fromPointer, toPointer) \ ( \ AssertMacro(PointerIsValid(toPointer)), \ AssertMacro(PointerIsValid(fromPointer)), \ *(toPointer) = *(fromPointer) \ ) regards, Ranier Vilela
Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
Also, in: 5. Line 2671 (nbtutils.c): ItemPointerGetBlockNumber(BTreeTupleGetHeapTID(newtup)), ItemPointerGetOffsetNumber(BTreeTupleGetHeapTID(newtup)), itemptr.h: /* * ItemPointerGetBlockNumberNoCheck * Returns the block number of a disk item pointer. */ #define ItemPointerGetBlockNumberNoCheck(pointer) \ ( \ BlockIdGetBlockNumber(&(pointer)->ip_blkid) \ ) #define ItemPointerGetOffsetNumberNoCheck(pointer) \ ( \ (pointer)->ip_posid \ ) regards, Ranier Vilela
Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
> Even if BTreeTupleGetHeapTID() did somehow return a NULL > pointer, then the user would be getting off lightly by experiencing a > hard crash instead of data corruption. > Oh, I'm sorry, I thought that "hard crash" was a bad thing. Ranier Vilela
Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
Em qua., 2 de set. de 2020 às 20:17, Peter Geoghegan escreveu: > On Wed, Sep 2, 2020 at 3:16 PM Ranier Vilela wrote: > Perhaps you recall our discussion of a similar false positive in > nbtsplitloc.c; that had a similar feel to it. For example, if your > static analysis tool says that code that has been around for many > years is riddled with bugs, then the problem is almost certainly with > the tool (or with how the tool has been used). > I'm using about 4 static analysis tools. Only one of them has a report of 67690 pages. Assuming an optimistic trend, only 1% could be real failures. It would have 670 real flaws in the entire code. In addition, if the code is compiled with -Wextra, hundreds of type mismatch alerts will be reported, especially comparison between int (signed) and unsigned types. If you try to leaks, you will find this: == 42483 == ERROR: LeakSanitizer: detected memory leaks Direct leak of 576 byte (s) in 1 object (s) allocated from: # 0 0x7f3204fb7bc8 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10dbc8) # 1 0x561c58d5b766 in save_ps_display_args /usr/src/postgres/src/backend/utils/misc/ps_status.c:178 # 2 0x561c584a90b3 in main /usr/src/postgres/src/backend/main/main.c:89 # 3 0x7f3204b7f0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2) Indirect leak of 14 byte (s) in 1 object (s) allocated from: # 0 0x7f3204f403dd in strdup (/lib/x86_64-linux-gnu/libasan.so.5+0x963dd) # 1 0x561c58d5b817 in save_ps_display_args /usr/src/postgres/src/backend/utils/misc/ps_status.c:186 # 2 0x561c584a90b3 in main /usr/src/postgres/src/backend/main/main.c:89 # 3 0x7f3204b7f0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2) For each reported failure, I have analyzed the code, without focusing on the "intention of the code", but trying to abstract and concentrate on following the flow of the variables to make sure that the alert is invalid. Maybe I didn't realize it, but I'm not "getting out of my head", these alerts. > > High performance C code very often relies on representational > invariants that may not be readily apparent to a compiler -- > especially when dealing with on-disk state. Barring some obvious > problem like a hard crash, you have to do the work of assessing if the > code is correct based on the actual assumptions/context of the code. A > static analysis tool is of very little use if you're not going to put > the work in. I'm sure that all these problems will be solved in the future. But how many others will be added. I have realized that maybe, it may be hindering the development course, so I decided to stop. I went to a lot of trouble to clearly document the code > in question here (the heap TID stuff in _bt_truncate()) -- did you > even bother to read those comments once? > Yes, I read. Maybe in this specific case, only this, it would solve for the code and the static analysis tool, but it's just a thought. pivotheaptid = (ItemPointer) ((char *) tidpivot + IndexTupleSize(tidpivot) - sizeof(ItemPointerData)); Anyway, thanks for all the answers. Ranier Vilela
Re: [PATCH] Redudant initilization
Em qui., 3 de set. de 2020 às 23:57, Bruce Momjian escreveu: > On Wed, Apr 1, 2020 at 08:57:18AM -0300, Ranier Vilela wrote: > > Hi, > > New patch with yours suggestions. > > Patch applied to head, thanks. > Thank you Bruce. regards, Ranier Vilela
Re: [PATCH] Redudant initilization
Em sex., 4 de set. de 2020 às 11:01, Bruce Momjian escreveu: > On Fri, Sep 4, 2020 at 09:39:45AM -0300, Ranier Vilela wrote: > > Em qui., 3 de set. de 2020 às 23:57, Bruce Momjian > escreveu: > > > > On Wed, Apr 1, 2020 at 08:57:18AM -0300, Ranier Vilela wrote: > > > Hi, > > > New patch with yours suggestions. > > > > Patch applied to head, thanks. > > > > Thank you Bruce. > > I have to say, I am kind of stumped why compilers do not warn of such > cases, and why we haven't gotten reports about these cases before. > I believe it is because, syntactically, there is no error. I would like to thank Kyotaro Horiguchi, my thanks for your review. regards, Ranier Vilela
Re: [PATCH] Redudant initilization
Em sex., 4 de set. de 2020 às 14:40, Tom Lane escreveu: > Bruce Momjian writes: > > I have to say, I am kind of stumped why compilers do not warn of such > > cases, and why we haven't gotten reports about these cases before. > > I was just experimenting with clang's "scan-build" tool. It finds > all of the cases you just fixed, and several dozen more beside. > Quite a few are things that, as a matter of style, we should *not* > change, for instance > > rewriteHandler.c:2807:5: warning: Value stored to 'outer_reloids' is never > read > outer_reloids = > list_delete_last(outer_reloids); > ^ > ~~~ > There are some like this, in the analyzes that I did. Even when it is the last action of the function. > Failing to update the list pointer here would just be asking for bugs. > However, I see some that look like genuine oversights; will go fix. > Thanks for fixing this. > (I'm not sure how much I trust scan-build overall. It produces a > whole bunch of complaints about null pointer dereferences, for instance. > If those aren't 99% false positives, we'd be crashing constantly. > It's also dog-slow. But it might be something to try occasionally.) > I believe it would be very beneficial. Attached is a patch I made in March/2020, but due to problems, it was sent but did not make the list. Would you mind taking a look? Certainly, if accepted, rebasing would have to be done. regards, Ranier Vilela variables_assigned_unused_value.patch Description: Binary data
Re: [PATCH] Redudant initilization
Em sex., 4 de set. de 2020 às 18:20, Ranier Vilela escreveu: > Em sex., 4 de set. de 2020 às 14:40, Tom Lane > escreveu: > >> Bruce Momjian writes: >> > I have to say, I am kind of stumped why compilers do not warn of such >> > cases, and why we haven't gotten reports about these cases before. >> >> I was just experimenting with clang's "scan-build" tool. It finds >> all of the cases you just fixed, and several dozen more beside. >> Quite a few are things that, as a matter of style, we should *not* >> change, for instance >> >> rewriteHandler.c:2807:5: warning: Value stored to 'outer_reloids' is >> never read >> outer_reloids = >> list_delete_last(outer_reloids); >> ^ >> ~~~ >> > There are some like this, in the analyzes that I did. > Even when it is the last action of the function. > > >> Failing to update the list pointer here would just be asking for bugs. >> However, I see some that look like genuine oversights; will go fix. >> > Thanks for fixing this. > > >> (I'm not sure how much I trust scan-build overall. It produces a >> whole bunch of complaints about null pointer dereferences, for instance. >> If those aren't 99% false positives, we'd be crashing constantly. >> It's also dog-slow. But it might be something to try occasionally.) >> > I believe it would be very beneficial. > > Attached is a patch I made in March/2020, but due to problems, > it was sent but did not make the list. > Would you mind taking a look? > Certainly, if accepted, rebasing would have to be done. > Here it is simplified, splitted and rebased. Some are bogus, others are interesting. regards, Ranier Vilela fix_redudant_initialization_firstmissingnum_heaptuple.patch Description: Binary data fix_redudant_initialization_offsetnumber_gistutil.patch Description: Binary data fix_redudant_initialization_formatting.patch Description: Binary data fix_redudant_initialization_arrayfuncs.patch Description: Binary data fix_redudant_initialization_bklno_hash.patch Description: Binary data fix_redudant_initialization_parse_utilcmd.patch Description: Binary data fix_redudant_initialization_procarray.patch Description: Binary data fix_redudant_initialization_status_nbtsearch.patch Description: Binary data fix_redudant_initialization_storage.patch Description: Binary data fix_redudant_initialization_spell.patch Description: Binary data fix_redudant_initialization_syslogger.patch Description: Binary data fix_redudant_initialization_to_tsany.patch Description: Binary data fix_redudant_initialization_tsrank.patch Description: Binary data fix_redudant_initialization_tuplesort.patch Description: Binary data fix_redudant_initialization_wparser_def.patch Description: Binary data fix_redudant_prefix_spgtextproc.patch Description: Binary data fix_redudant_waits_xlog.patch Description: Binary data
Re: [PATCH] Redudant initilization
Em sáb., 5 de set. de 2020 às 14:29, Tom Lane escreveu: > Ranier Vilela writes: > > Attached is a patch I made in March/2020, but due to problems, > > it was sent but did not make the list. > > Would you mind taking a look? > > I applied some of this, but other parts had been overtaken by > events, and there were other changes that I didn't agree with. > I fully agree with your judgment. > A general comment on the sort of "dead store" that I don't think > we should remove is where a function is trying to maintain an > internal invariant, such as "this pointer points past the last > data written to a buffer" or "these two variables are in sync". > If the update happens to be the last one in the function, the > compiler may be able to see that the store is dead ... but IMO > it should just optimize such a store away and not get in the > programmer's face about it. If we manually remove the dead > store then what we've done is broken the invariant, and we'll > pay for that in future bugs and maintenance costs. Somebody > may someday want to add more code after the step in question, > and if they fail to undo the manual optimization then they've > got a bug. Besides which, it's confusing when a function > does something the same way N-1 times and then differently the N'th time. > Good point. The last store is a little strange, but the compiler will certainly optimize. Maintenance is expensive, and the current code should be the best example. regards, Ranier Vilela
Re: A micro-optimisation for walkdir()
Hi Juan,
This is only a suggestion, if you find it appropriate.
We could use a little cut tail in get_dirent_type function.
Try to avoid add padding, when modifying or adding fields.
struct dirent
{
long d_ino;
unsigned short d_reclen;
unsigned short d_namlen;
+ unsigned char d_type;
char d_name[MAX_PATH];
};
Or even better if possible:
struct dirent
{
char d_name[MAX_PATH];
long d_ino;
unsigned short d_reclen;
unsigned short d_namlen;
unsigned char d_type;
};
regards,
Ranier Vilela
v4-0001-Skip-unnecessary-stat-calls-in-walkdir.patch
Description: Binary data
v4-0002-Add-d_type-to-WIN32-dirent-port.patch
Description: Binary data
Re: Windows regress fails (latest HEAD)
Em sex., 26 de jun. de 2020 às 08:21, Ranier Vilela escreveu: > Em qui., 11 de jun. de 2020 às 10:28, Ranier Vilela > escreveu: > >> Em qui., 11 de jun. de 2020 às 10:01, Andrew Dunstan < >> andrew.duns...@2ndquadrant.com> escreveu: >> >>> >>> On 6/11/20 8:52 AM, Ranier Vilela wrote: >>> > Hi, >>> > Latest HEAD, fails with windows regress tests. >>> > >>> > float8 ... FAILED 517 ms >>> > partition_prune ... FAILED 3085 ms >>> > >>> > >>> >>> The first thing you should do when you find this is to see if there is a >>> buildfarm report of the failure. If there isn't then try to work out why. >>> >> Sorry, I will have to research the buildfarm, I have no reference to it. >> >> >>> >>> Also, when making a report like this, it is essential to let us know >>> things like: >>> >>> * which commit is causing the failure (git bisect is good for finding >>> this) >>> >> Thanks for hit (git bisect). >> >> >>> * what Windows version you're testing on >>> >> Windows 10 (2004) >> >> * which compiler you're using >>> >> msvc 2019 (64 bits) >> > > Only for registry, if anyone else is using msvc 2019. > I'm using latest msvc 2019 64 bits (16.6.0) > Problably this is a compiler optimization bug. > vcregress check with build DEBUG, pass all 200 tests. > With the current HEAD, the regression float8 in release mode (msvc 2019 64 bits) is gone. Maybe it's this commit: https://github.com/postgres/postgres/commit/0aa8f764088ea0f36620ae2955fa6c54ec736c46 But (partition_prune) persists. partition_prune ... FAILED regards, Ranier Vilela
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Em ter., 2 de nov. de 2021 às 15:33, Andres Freund escreveu: > On 2021-11-02 13:43:46 -0400, Tom Lane wrote: > > Ranier Vilela writes: > > > It seems that 1ec7679f1b67e84be688a311dce234eeaa1d5de8 caused the > problem. > > > > Indeed. Fix pushed. > > Thanks to both of you! > You are welcome, Andres. regards, Ranier Vilela
[BUG FIX] Uninitialized var fargtypes used.
Hi,
Can anyone check this bug fix?
Thanks.
Ranier Vilela
--- \dll\postgresql-12.0\a\backend\commands\event_trigger.c Mon Sep 30
17:06:55 2019
+++ event_trigger.c Mon Nov 11 13:52:35 2019
@@ -171,7 +171,7 @@
HeapTuple tuple;
Oid funcoid;
Oid funcrettype;
- Oid fargtypes[1]; /* dummy */
+ Oid fargtypes[1] = {InvalidOid, InvalidOid};
/* dummy */
Oid evtowner = GetUserId();
ListCell *lc;
List *tags = NULL;
event_trigger.c.patch
Description: event_trigger.c.patch
RE: [BUG FIX] Uninitialized var fargtypes used.
Hi,
Sorry by error in the patch.
--- \dll\postgresql-12.0\a\backend\commands\event_trigger.c Mon Sep 30 17:06:55
2019
+++ event_trigger.c Tue Nov 12 08:34:30 2019
@@ -171,7 +171,7 @@
HeapTuple tuple;
Oid funcoid;
Oid funcrettype;
- Oid fargtypes[1]; /* dummy */
+ Oid fargtypes[1] = {InvalidOid}; /* dummy */
Oid evtowner = GetUserId();
ListCell *lc;
List *tags = NULL;
De: Michael Paquier
Enviado: terça-feira, 12 de novembro de 2019 03:31
Para: Ranier Vilela
Cc: pgsql-hackers@lists.postgresql.org
Assunto: Re: [BUG FIX] Uninitialized var fargtypes used.
On Mon, Nov 11, 2019 at 06:28:47PM +, Ranier Vilela wrote:
> Can anyone check this bug fix?
>
> +++ event_trigger.c Mon Nov 11 13:52:35 2019
> @@ -171,7 +171,7 @@
> HeapTuple tuple;
> Oid funcoid;
> Oid funcrettype;
> - Oid fargtypes[1]; /* dummy */
> + Oid fargtypes[1] = {InvalidOid, InvalidOid};
> /* dummy */
> Oid evtowner = GetUserId();
Yeah, it would be better to fix this initialization.
--
Michael
event_trigger.c.patch
Description: event_trigger.c.patch
[PATCH][BUG_FIX] Potential null pointer dereferencing.
Hi,
ExecClearTuple don't check por NULL pointer arg and according
TupIsNull slot can be NULL.
Can anyone check this buf fix?
--- \dll\postgresql-12.0\a\backend\executor\nodeUnique.cMon Sep 30
17:06:55 2019
+++ nodeUnique.cTue Nov 12 09:54:34 2019
@@ -74,7 +74,8 @@
if (TupIsNull(slot))
{
/* end of subplan, so we're done */
- ExecClearTuple(resultTupleSlot);
+ if (!TupIsNull(resultTupleSlot))
+ ExecClearTuple(resultTupleSlot);
return NULL;
}
nodeUnique.c.patch
Description: nodeUnique.c.patch
RE: [PATCH][BUG_FIX] Potential null pointer dereferencing.
Hi, The condition is : 74. if (TupIsNull(slot)) is true 85. if (TupIsNull(resultTupleSlot)) is true too. If resultTupleSlot is not can be NULL, why test if (TupIsNull(resultTupleSlot))? Occurring these two conditions ExecClearTuple is called, but, don't check by NULL arg. There are at least 2 more possible cases, envolving ExecClearTuple: nodeFunctionscan.c and nodeWindowAgg.c Best regards, Ranier Vilela De: Daniel Gustafsson Enviado: terça-feira, 12 de novembro de 2019 13:43 Para: Ranier Vilela Cc: PostgreSQL Hackers Assunto: Re: [PATCH][BUG_FIX] Potential null pointer dereferencing. > On 12 Nov 2019, at 14:07, Ranier Vilela wrote: > ExecClearTuple don't check por NULL pointer arg and according > TupIsNull slot can be NULL. I assume you are referring to the TupIsNull(resultTupleSlot) check a few lines down in the same loop? If resultTupleSlot was indeed NULL and not empty, the subsequent call to ExecCopySlot would be a NULL pointer dereference too. I might be missing something obvious, but in which case can resultTupleSlot be NULL when calling ExecUnique? cheers ./daniel
[PATCH][BUG FIX] Potential uninitialized vars used.
Hi,
Var TargetEntry *tle;
Have several paths where can it fail.
Can anyone check this bug fix?
--- \dll\postgresql-12.0\a\backend\parser\parse_expr.c Mon Sep 30 17:06:55 2019
+++ parse_expr.cTue Nov 12 12:43:07 2019
@@ -349,6 +349,7 @@
errmsg("DEFAULT is not allowed in this
context"),
parser_errposition(pstate,
((SetToDefault *) expr)->location)));
+ result = NULL; /* keep compiler quiet */
break;
/*
@@ -1637,11 +1638,13 @@
pstate->p_multiassign_exprs =
lappend(pstate->p_multiassign_exprs,
tle);
}
- else
+ else {
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("source for a multiple-column
UPDATE item must be a sub-SELECT or ROW() expression"),
parser_errposition(pstate,
exprLocation(maref->source;
+return NULL;
+}
}
else
{
@@ -1653,6 +1656,10 @@
Assert(pstate->p_multiassign_exprs != NIL);
tle = (TargetEntry *) llast(pstate->p_multiassign_exprs);
}
+if (tle == NULL) {
+ elog(ERROR, "unexpected expr type in multiassign list");
+ return NULL;/* keep compiler quiet
*/
+}
/*
* Emit the appropriate output expression for the current column
parse_expr.c.patch
Description: parse_expr.c.patch
[PATCH] Improve AtSubCommit_childXids
Hi,
Surely that "s->nChildXids > 0", protects s->childXids to be NULL!
But, when we exchange the test (s->nChildXids > 0) by (s->childXids != NULL), I
believe we have the same protection, because, if "s->childXids" is not NULL,
"s->nChildXids" is > 0, naturally.
That way we can improve the function and avoid calling and setting
unnecessarily!
Bonus: silent compiler warning potential null pointer derenferencing.
Best regards,
Ranier Vilela
--- \dll\postgresql-12.0\a\backend\access\transam\xact.cMon Sep 30
17:06:55 2019
+++ xact.c Wed Nov 13 13:03:28 2019
@@ -1580,20 +1580,20 @@
*/
s->parent->childXids[s->parent->nChildXids] =
XidFromFullTransactionId(s->fullTransactionId);
- if (s->nChildXids > 0)
+ if (s->childXids != NULL) {
memcpy(&s->parent->childXids[s->parent->nChildXids + 1],
s->childXids,
s->nChildXids * sizeof(TransactionId));
+ /* Release child's array to avoid leakage */
+pfree(s->childXids);
+ /* We must reset these to avoid double-free if fail later in commit
*/
+ s->childXids = NULL;
+ s->nChildXids = 0;
+ s->maxChildXids = 0;
+}
+ Assert(s->nChildXids == 0 && s->maxChildXids == 0);
s->parent->nChildXids = new_nChildXids;
-
- /* Release child's array to avoid leakage */
- if (s->childXids != NULL)
- pfree(s->childXids);
- /* We must reset these to avoid double-free if fail later in commit */
- s->childXids = NULL;
- s->nChildXids = 0;
- s->maxChildXids = 0;
}
/*
xact.c.patch
Description: xact.c.patch
RE: [PATCH] Improve AtSubCommit_childXids
"Why is this an improvement? And what setting are we removing? You mean that we reset nChildXids, even if it's already 0? Hard to see how that matters." The orginal function, ever set ChildXidsm, nChildXidsa and maxChildXids. See at lines 1594, 1595, 1596, even if it's already 0! The test (nChildXids > 0), possibly works, but, may confuse when do use memcpy function soon after, and access one pointer that below, is checked by NULL. How hard to see this? Original file: if (s->nChildXids > 0) memcpy(&s->parent->childXids[s->parent->nChildXids + 1], s->childXids, // s->childXids null pointer potential dereferencing s->nChildXids * sizeof(TransactionId)); s->parent->nChildXids = new_nChildXids; /* Release child's array to avoid leakage */ if (s->childXids != NULL) // Check null pointer! pfree(s->childXids); /* We must reset these to avoid double-free if fail later in commit */ s->childXids = NULL; // ever set to 0 s->nChildXids = 0; // ever set to 0 s->maxChildXids = 0; // ever set to 0 best regards, Ranier Vilela ____ De: Andres Freund Enviado: quarta-feira, 13 de novembro de 2019 17:10 Para: Ranier Vilela Cc: PostgreSQL Hackers Assunto: Re: [PATCH] Improve AtSubCommit_childXids Hi, On 2019-11-13 16:18:46 +, Ranier Vilela wrote: > Surely that "s->nChildXids > 0", protects s->childXids to be NULL! > But, when we exchange the test (s->nChildXids > 0) by (s->childXids != NULL), > I believe we have the same protection, because, if "s->childXids" is not > NULL, "s->nChildXids" is > 0, naturally. > > That way we can improve the function and avoid calling and setting > unnecessarily! Why is this an improvement? And what setting are we removing? You mean that we reset nChildXids, even if it's already 0? Hard to see how that matters. > Bonus: silent compiler warning potential null pointer derenferencing. Which compiler issues a warning here? Greetings, Andres Freund
[PATCH][BUG FIX] Unsafe access pointers.
Hi,
Last time, I promise.
It's probably not happening, but it can happen, I think.
Best regards.
Ranier Vilela
--- \dll\postgresql-12.0\a\backend\access\brin\brin_validate.c Mon Sep 30
17:06:55 2019
+++ brin_validate.c Fri Nov 15 08:14:58 2019
@@ -57,8 +57,10 @@
/* Fetch opclass information */
classtup = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclassoid));
- if (!HeapTupleIsValid(classtup))
+ if (!HeapTupleIsValid(classtup)) {
elog(ERROR, "cache lookup failed for operator class %u",
opclassoid);
+return false;
+}
classform = (Form_pg_opclass) GETSTRUCT(classtup);
opfamilyoid = classform->opcfamily;
@@ -67,8 +69,11 @@
/* Fetch opfamily information */
familytup = SearchSysCache1(OPFAMILYOID, ObjectIdGetDatum(opfamilyoid));
- if (!HeapTupleIsValid(familytup))
+ if (!HeapTupleIsValid(familytup)) {
elog(ERROR, "cache lookup failed for operator family %u",
opfamilyoid);
+ ReleaseSysCache(classtup);
+return false;
+}
familyform = (Form_pg_opfamily) GETSTRUCT(familytup);
opfamilyname = NameStr(familyform->opfname);
brin_validate.c.patch
Description: brin_validate.c.patch
RE: [PATCH][BUG FIX] Unsafe access pointers.
Hi,
Thank you for the explanation.
Best regards.
Ranier Vilela
De: Daniel Gustafsson
Enviado: sexta-feira, 15 de novembro de 2019 11:58
Para: Ranier Vilela
Cc: pgsql-hackers@lists.postgresql.org
Assunto: Re: [PATCH][BUG FIX] Unsafe access pointers.
> On 15 Nov 2019, at 12:25, Ranier Vilela wrote:
> It's probably not happening, but it can happen, I think.
I don't think it can, given how elog() works.
> - if (!HeapTupleIsValid(classtup))
> + if (!HeapTupleIsValid(classtup)) {
> elog(ERROR, "cache lookup failed for operator class %u",
> opclassoid);
> +return false;
elog or ereport with a severity of ERROR or higher will never return.
> - if (!HeapTupleIsValid(familytup))
> + if (!HeapTupleIsValid(familytup)) {
> elog(ERROR, "cache lookup failed for operator family %u",
> opfamilyoid);
> + ReleaseSysCache(classtup);
> +return false;
> +}
Not only will elog(ERROR ..) not return to run this, the errorhandling
machinery will automatically release resources and clean up.
cheers ./daniel
[PATCH][BUG FIX] Uninitialized variable parsed
Hi,
Typo mystake?
Memset only fill a pointer size, not the size of struct.
Best regards.
Ranier Vilela
--- \dll\postgresql-12.0\a\backend\access\rmgrdesc\xactdesc.c Mon Sep 30
17:06:55 2019
+++ xactdesc.c Fri Nov 22 13:40:13 2019
@@ -35,7 +35,7 @@
{
char *data = ((char *) xlrec) + MinSizeOfXactCommit;
- memset(parsed, 0, sizeof(*parsed));
+ memset(parsed, 0, sizeof(xl_xact_parsed_commit));
parsed->xinfo = 0; /* default, if no
XLOG_XACT_HAS_INFO is
* present */
@@ -130,7 +130,7 @@
{
char *data = ((char *) xlrec) + MinSizeOfXactAbort;
- memset(parsed, 0, sizeof(*parsed));
+ memset(parsed, 0, sizeof(xl_xact_parsed_commit));
parsed->xinfo = 0; /* default, if no
XLOG_XACT_HAS_INFO is
* present */
xactdesc.c.patch
Description: xactdesc.c.patch
[PATCH][BUG FIX] Pointer arithmetic with NULL
Hi,
Pointer addition with NULL, is technically undefined behavior.
Best regards.
Ranier Vilela
--- \dll\postgresql-12.0\a\backend\access\transam\xlog.cMon Sep 30
17:06:55 2019
+++ xlog.c Fri Nov 22 13:57:17 2019
@@ -1861,7 +1861,7 @@
{
Assert(((XLogPageHeader) cachedPos)->xlp_magic ==
XLOG_PAGE_MAGIC);
Assert(((XLogPageHeader) cachedPos)->xlp_pageaddr == ptr - (ptr
% XLOG_BLCKSZ));
- return cachedPos + ptr % XLOG_BLCKSZ;
+ return ptr % XLOG_BLCKSZ;
}
/*
xlog.c.patch
Description: xlog.c.patch
[PATCH][BUG FIX] Pointer var initilialized with boolean.
Hi,
Typo mystake?
Pointer var initilialized with boolean.
Best regards.
Ranier Vilela
--- \dll\postgresql-12.0\a\backend\commands\trigger.c Mon Sep 30 17:06:55 2019
+++ trigger.c Fri Nov 22 14:20:56 2019
@@ -2536,7 +2536,7 @@
TupleTableSlot *slot)
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
- HeapTuple newtuple = false;
+ HeapTuple newtuple = NULL;
boolshould_free;
TriggerData LocTriggerData;
int i;
@@ -3178,7 +3178,7 @@
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TupleTableSlot *oldslot = ExecGetTriggerOldSlot(estate, relinfo);
- HeapTuple newtuple = false;
+ HeapTuple newtuple = NULL;
boolshould_free;
TriggerData LocTriggerData;
int i;
trigger.c.patch
Description: trigger.c.patch
[PATCH] Tiny optmization.
Hi, Maybe it doesn't matter, but, I think it's worth discussing. The expression "if(pstate)" is redundant or is possible null dereference. Best regards. Ranier Vilela --- \dll\postgresql-12.0\a\backend\commands\copy.c Mon Sep 30 17:06:55 2019 +++ copy.c Fri Nov 22 18:33:05 2019 @@ -3426,8 +3426,7 @@ cstate->raw_buf_index = cstate->raw_buf_len = 0; /* Assign range table, we'll need it in CopyFrom. */ - if (pstate) - cstate->range_table = pstate->p_rtable; + cstate->range_table = pstate->p_rtable; tupDesc = RelationGetDescr(cstate->rel); num_phys_attrs = tupDesc->natts; copy.c.patch Description: copy.c.patch
[PATCH] Tiny optmization or is a bug?
Hi, Maybe this is a real bug. The assignment has no effect, or forget dereferencing it? Best regards. Ranier Vilela --- \dll\postgresql-12.0\a\backend\commands\lockcmds.c Mon Sep 30 17:06:55 2019 +++ lockcmds.c Fri Nov 22 18:45:01 2019 @@ -285,7 +285,7 @@ LockViewRecurse_walker((Node *) viewquery, &context); - ancestor_views = list_delete_oid(ancestor_views, reloid); + list_delete_oid(ancestor_views, reloid); table_close(view, NoLock); } lockcmds.c.patch Description: lockcmds.c.patch
[PATCH] Tiny optimization.
Hi,
Remove redutant test.
best regards.
Ranier Vilela
--- \dll\postgresql-12.0\a\backend\executor\execExpr.c Mon Sep 30 17:06:55 2019
+++ execExpr.c Fri Nov 22 18:50:32 2019
@@ -2426,7 +2426,7 @@
{
desc = parent->scandesc;
- if (parent && parent->scanops)
+ if (parent->scanops)
tts_ops = parent->scanops;
if (parent->scanopsset)
execExpr.c.patch
Description: execExpr.c.patch
RE: [PATCH] Tiny optmization.
Hi, Redudant because he it's been dereferenced here: line 3410: cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options); Best regards. Ranier Vilela De: Tomas Vondra Enviado: sexta-feira, 22 de novembro de 2019 22:05 Para: Ranier Vilela Cc: pgsql-hack...@postgresql.org Assunto: Re: [PATCH] Tiny optmization. On Fri, Nov 22, 2019 at 09:41:44PM +0000, Ranier Vilela wrote: >Hi, >Maybe it doesn't matter, but, I think it's worth discussing. >The expression "if(pstate)" is redundant or is possible null dereference. > Eh? Redundant with what? Why would it be a null dereference? It's a parameter passed from outside, and we're not checking it before. And the if condition is there exactly to prevent null dereference. It's generally a good idea to inspect existing callers of the modified function and try running tests before submitting a patch. In this case there's a BeginCopyFrom() call in contrib/file_fdw, passing NULL as the first parameter, and if you run `make check` for that module it falls flat on it's face due to a segfault. regards > >--- \dll\postgresql-12.0\a\backend\commands\copy.c Mon Sep 30 17:06:55 2019 >+++ copy.c Fri Nov 22 18:33:05 2019 >@@ -3426,8 +3426,7 @@ > cstate->raw_buf_index = cstate->raw_buf_len = 0; > > /* Assign range table, we'll need it in CopyFrom. */ >- if (pstate) >- cstate->range_table = pstate->p_rtable; >+ cstate->range_table = pstate->p_rtable; > > tupDesc = RelationGetDescr(cstate->rel); > num_phys_attrs = tupDesc->natts; -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
RE: [PATCH] Tiny optmization.
Hi,
pstate is touched here:
a) BeginCopy line 1489:
ProcessCopyOptions(pstate, cstate, is_from, options);
b) ProcessCopyOptions line 1137:
if (format_specified)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or
redundant options"),
parser_errposition(pstate,
defel->location)));
best regards.
Ranier Vilela
De: Tom Lane
Enviado: sexta-feira, 22 de novembro de 2019 22:17
Para: Ranier Vilela
Cc: pgsql-hack...@postgresql.org
Assunto: Re: [PATCH] Tiny optmization.
Ranier Vilela writes:
> Redudant because he it's been dereferenced here:
> line 3410:
> cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist,
> options);
Not necessarily ... the rel!=NULL code path there doesn't touch pstate,
and that seems to be what contrib/file_fdw is relying on.
Arguably, the rel==NULL code path in BeginCopy should be prepared to
support pstate being null, too. But what you proposed here is certainly
not OK.
regards, tom lane
[BUG] (firsttupleslot)==NULL is redundant or is possible null dereference?
Hi,
This is real bug? firsttupleslot == NULL.
\backend\executor\nodeGroup.c
if (TupIsNull(firsttupleslot))
{
outerslot = ExecProcNode(outerPlanState(node));
if (TupIsNull(outerslot))
{
/* empty input, so return nothing */
node->grp_done = true;
return NULL;
}
/* Copy tuple into firsttupleslot */
ExecCopySlot(firsttupleslot, outerslot);
include\executor\tuptable.h:
#define TupIsNull(slot) \
((slot) == NULL || TTS_EMPTY(slot))
static inline TupleTableSlot *
ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
{
Assert(!TTS_EMPTY(srcslot));
dstslot->tts_ops->copyslot(dstslot, srcslot);
return dstslot;
}
RE: [BUG] (firsttupleslot)==NULL is redundant or is possible null dereference?
Hi,
Sorry, you are right.
Had not seen this line:
firsttupleslot = node->ss.ss_ScanTupleSlot;
Best regards.
Ranier Vilela
De: Tomas Vondra
Enviado: sexta-feira, 22 de novembro de 2019 22:54
Para: Ranier Vilela
Cc: pgsql-hack...@postgresql.org
Assunto: Re: [BUG] (firsttupleslot)==NULL is redundant or is possible null
dereference?
On Fri, Nov 22, 2019 at 10:32:11PM +0000, Ranier Vilela wrote:
>Hi,
>This is real bug? firsttupleslot == NULL.
>
Ranier, I don't want to be rude, but I personally am getting a bit
annoyed by this torrent of bug reports that are essentially just a bunch
of copy-pasted chunks of code, without any specification of bench,
position in the file, etc.
And more importantly, without any clear explanation why you think it is
a bug (or even a demonstration of an issue), and "Is this a bug?"
>\backend\executor\nodeGroup.c
> if (TupIsNull(firsttupleslot))
> {
> outerslot = ExecProcNode(outerPlanState(node));
> if (TupIsNull(outerslot))
> {
> /* empty input, so return nothing */
> node->grp_done = true;
> return NULL;
> }
> /* Copy tuple into firsttupleslot */
> ExecCopySlot(firsttupleslot, outerslot);
>
>include\executor\tuptable.h:
>#define TupIsNull(slot) \
> ((slot) == NULL || TTS_EMPTY(slot))
>
>static inline TupleTableSlot *
>ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
>{
> Assert(!TTS_EMPTY(srcslot));
>
> dstslot->tts_ops->copyslot(dstslot, srcslot);
>
> return dstslot;
>}
>
And why do you think this is a bug? Immediately before the part of code
you copied we have this:
/*
* The ScanTupleSlot holds the (copied) first tuple of each group.
*/
firsttupleslot = node->ss.ss_ScanTupleSlot;
And node->ss.ss_ScanTupleSlot is expected to be non-NULL. So the initial
assumption that firsttupleslot is NULL is incorrect.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
