Thread: Possible cache reference leak by removeExtObjInitPriv

Hello.

Recently a cache reference leak was reported then fixed [1].

I happened to notice a similar possible leakage in
removeEtObjInitPriv. I haven't found a way to reach the code, but can
be forcibly caused by tweaking the condition.

Please find the attached.

regards.

[1]
https://www.postgresql.org/message-id/BYAPR08MB5606D1453D7F50E2AF4D2FD29AD80@BYAPR08MB5606.namprd08.prod.outlook.com

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index a3f680d388..11d9de9031 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -5869,11 +5869,17 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
         /* Indexes don't have permissions */
         if (pg_class_tuple->relkind == RELKIND_INDEX ||
             pg_class_tuple->relkind == RELKIND_PARTITIONED_INDEX)
+        {
+            ReleaseSysCache(tuple);
             return;
+        }
 
         /* Composite types don't have permissions either */
         if (pg_class_tuple->relkind == RELKIND_COMPOSITE_TYPE)
+        {
+            ReleaseSysCache(tuple);
             return;
+        }
 
         /*
          * If this isn't a sequence, index, or composite type then it's

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> Recently a cache reference leak was reported then fixed [1].
> I happened to notice a similar possible leakage in
> removeEtObjInitPriv. I haven't found a way to reach the code, but can
> be forcibly caused by tweaking the condition.
> Please find the attached.

Ugh.  recordExtObjInitPriv has the same problem.

I wonder whether there is any way to teach Coverity, or some other
static analyzer, to look for code paths that leak cache refcounts.
It seems isomorphic to detecting memory leaks, which Coverity is
reasonably good at.

            regards, tom lane

At Fri, 17 Apr 2020 13:07:15 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > Recently a cache reference leak was reported then fixed [1].
> > I happened to notice a similar possible leakage in
> > removeEtObjInitPriv. I haven't found a way to reach the code, but can
> > be forcibly caused by tweaking the condition.
> > Please find the attached.
> 
> Ugh.  recordExtObjInitPriv has the same problem.

Thanks for commit it.

> I wonder whether there is any way to teach Coverity, or some other
> static analyzer, to look for code paths that leak cache refcounts.
> It seems isomorphic to detecting memory leaks, which Coverity is
> reasonably good at.

Indeed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 23 Apr 2020 01:21:21 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Em qua., 22 de abr. de 2020 às 23:27, Kyotaro Horiguchi <
> horikyota.ntt@gmail.com> escreveu:
> >
> > -       strncpy(sqlca->sqlerrm.sqlerrmc, message,
> > sizeof(sqlca->sqlerrm.sqlerrmc));
> > -       sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] = 0;
> > +       sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] =
> > '\0';
> > +       strncpy(sqlca->sqlerrm.sqlerrmc, message,
> > sizeof(sqlca->sqlerrm.sqlerrmc) - 1);
> >
> > The existing strncpy then terminating by NUL works fine. I don't think
> > there's any point in doing the reverse way.  Actually
> > sizeof(sqlca->sqlerrm.sqlerrmc) - 1 is enough for the length but the
> > existing code is not necessarily a bug.
> >
> Without understanding then, why Coveriy claims bug here.

Well, handling non-terminated strings with str* functions are a sign
of bug in most cases.  Coverity is very useful but false positives are
annoying.  I wonder what if we attach Coverity annotations to such
codes.

By the way, do you have some ideas of how to let coverity detect
leakage of resources other than memory?  We found several cases of
cache reference leakage that should be statically detected easily.

https://www.postgresql.org/message-id/10513.1587143235@sss.pgh.pa.us
> I wonder whether there is any way to teach Coverity, or some other
> static analyzer, to look for code paths that leak cache refcounts.
> It seems isomorphic to detecting memory leaks, which Coverity is
> reasonably good at.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

On 2020-04-23 14:36:15 +0900, Kyotaro Horiguchi wrote:
> At Thu, 23 Apr 2020 01:21:21 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> > Em qua., 22 de abr. de 2020 às 23:27, Kyotaro Horiguchi <
> > horikyota.ntt@gmail.com> escreveu:
> > >
> > > -       strncpy(sqlca->sqlerrm.sqlerrmc, message,
> > > sizeof(sqlca->sqlerrm.sqlerrmc));
> > > -       sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] = 0;
> > > +       sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] =
> > > '\0';
> > > +       strncpy(sqlca->sqlerrm.sqlerrmc, message,
> > > sizeof(sqlca->sqlerrm.sqlerrmc) - 1);
> > >
> > > The existing strncpy then terminating by NUL works fine. I don't think
> > > there's any point in doing the reverse way.  Actually
> > > sizeof(sqlca->sqlerrm.sqlerrmc) - 1 is enough for the length but the
> > > existing code is not necessarily a bug.
> > >
> > Without understanding then, why Coveriy claims bug here.
>
> Well, handling non-terminated strings with str* functions are a sign
> of bug in most cases.  Coverity is very useful but false positives are
> annoying.  I wonder what if we attach Coverity annotations to such
> codes.

It might be worth doing something about this, for other reasons. We have
disabled -Wstringop-truncation in 716585235b1. But I've enabled it in my
debug build, because I find it useful. The only warning we're getting
in non-optimized builds is

/home/andres/src/postgresql/src/interfaces/ecpg/ecpglib/misc.c: In function ‘ECPGset_var’:
/home/andres/src/postgresql/src/interfaces/ecpg/ecpglib/misc.c:565:17: warning: ‘strncpy’ output truncated before
terminatingnul copying 5 bytes from a string of the same length [-Wstringop-truncation] 
  565 |                 strncpy(sqlca->sqlstate, "YE001", sizeof(sqlca->sqlstate));
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

One way we could address this is to use the 'nonstring' attribute gcc
has introduced, signalling that sqlca_t->sqlstate isn't zero
terminated. That removes the above warning.

https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

"The nonstring variable attribute specifies that an object or member declaration with type array of char, signed char,
orunsigned char, or pointer to such a type is intended to store character arrays that do not necessarily contain a
terminatingNUL. This is useful in detecting uses of such arrays or pointers with functions that expect NUL-terminated
strings,and to avoid warnings when such an array or pointer is used as an argument to a bounded string manipulation
functionsuch as strncpy. For example, without the attribute, GCC will issue a warning for the strncpy call below
becauseit may truncate the copy without appending the terminating NUL character. Using the attribute makes it possible
tosuppress the warning. However, when the array is declared with the attribute the call to strlen is diagnosed because
whenthe array doesn’t contain a NUL-terminated string the call is undefined. To copy, compare, of search non-string
characterarrays use the memcpy, memcmp, memchr, and other functions that operate on arrays of bytes. In addition,
callingstrnlen and strndup with such arrays is safe provided a suitable bound is specified, and not diagnosed. " 

I've not looked at how much work it'd be to make a recent-ish gcc not to
produce lots of false positives in optimized builds.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> It might be worth doing something about this, for other reasons. We have
> disabled -Wstringop-truncation in 716585235b1. But I've enabled it in my
> debug build, because I find it useful.

ITYM e71658523 ?  I can't find that hash in my repo.  Anyway, I agree
that disabling that was a bit of a stopgap hack.  This 'nonstring'
attribute seems like it would help for ECPG's usage, at least.

> I've not looked at how much work it'd be to make a recent-ish gcc not to
> produce lots of false positives in optimized builds.

The discussion that led up to e71658523 seemed to conclude that the
only reasonable way to suppress the majority of those warnings was
to get rid of the fixed-length MAXPGPATH buffers we use everywhere.
Now that we have psprintf(), that might be more workable than before,
but the effort-to-reward ratio still doesn't seem promising.

            regards, tom lane

Andres Freund <andres@anarazel.de> writes:
> On 2021-06-11 19:08:57 -0400, Tom Lane wrote:
>> Anyway, I agree that disabling that was a bit of a stopgap hack.  This
>> 'nonstring' attribute seems like it would help for ECPG's usage, at
>> least.

> nonstring is supported since gcc 8, which also brought the warnings that
> e71658523 is concerned about. Which makes me think that we should be
> able to get away without a configure test. The one complication is that
> the relevant ecpg code doesn't include c.h.

Ugh.  And we *can't* include that there.

> But I think we can just do something like:

> -    char        sqlstate[5];
> +    char        sqlstate[5]
> +#if defined(__has_attribute) && __has_attribute(nonstring)
> +    __attribute__((nonstring))
> +#endif
> +    ;
>  };

Hmm.  Worth a try, anyway.

> Should we also include a pg_attribute_nonstring definition in c.h?
> Probably not, given that we right now don't have another user?

Yeah, no point till there's another use-case.  (I'm not sure
there ever will be, so I'm not excited about adding more
infrastructure than we have to.)

            regards, tom lane

Hi,

On 2021-06-15 07:40:46 -0300, Ranier Vilela wrote:
> memcpy would not suffer from it?

It'd not be correct for short sqlstates - you'd read beyond the end of
the source buffer. There are cases of it in the ecpg code.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2021-06-15 07:40:46 -0300, Ranier Vilela wrote:
>> memcpy would not suffer from it?

> It'd not be correct for short sqlstates - you'd read beyond the end of
> the source buffer. There are cases of it in the ecpg code.

What's a "short SQLSTATE"?  They're all five characters by definition.

            regards, tom lane

Hi,

On 2021-06-15 13:53:08 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2021-06-15 07:40:46 -0300, Ranier Vilela wrote:
> >> memcpy would not suffer from it?
> 
> > It'd not be correct for short sqlstates - you'd read beyond the end of
> > the source buffer. There are cases of it in the ecpg code.
> 
> What's a "short SQLSTATE"?  They're all five characters by definition.

I thought there were places that just dealt with "00" etc. And there are - but
it's just comparisons.

I still don't fully feel comfortable just using memcpy() though, given that
the sqlstates originate remotely / from libpq, making it hard to rely on the
fact that the buffer "ought to" always be at least 5 bytes long? As far as I
can tell there's no enforcement of PQresultErrorField(..., PG_DIAG_SQLSTATE)
being that long.

Greetings,

Andres Freund