[PATCH] Fix buffer not null terminated on (ecpg lib)

Hello.

At Wed, 22 Apr 2020 19:48:07 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in

Hi,
strncpy, it is not a safe function and has the risk of corrupting memory.
On ecpg lib, two sources, make use of strncpy risk, this patch tries to fix.

1. Make room for the last null-characte;
2. Copies Maximum number of characters - 1.

per Coverity.

-	strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate));
+	sqlca->sqlstate[sizeof(sqlca->sqlstate) - 1] = '\0';
+	strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate) - 1);

Did you look at the definition and usages of the struct member?
sqlstate is a char[5], which is to be filled with 5-letter SQLSTATE
code not terminated by NUL, which can be shorter if NUL is found
anywhere (I'm not sure there's actually a case of a shorter state
code). If you put NUL to the 5th element of the array, you break the
content. The existing code looks perfect to me.

-	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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Em qua., 22 de abr. de 2020 às 23:27, Kyotaro Horiguchi <
horikyota.ntt@gmail.com> escreveu:

Hello.

At Wed, 22 Apr 2020 19:48:07 -0300, Ranier Vilela <ranier.vf@gmail.com>
wrote in

Hi,
strncpy, it is not a safe function and has the risk of corrupting memory.
On ecpg lib, two sources, make use of strncpy risk, this patch tries to

fix.

1. Make room for the last null-characte;
2. Copies Maximum number of characters - 1.

per Coverity.

-       strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate));
+       sqlca->sqlstate[sizeof(sqlca->sqlstate) - 1] = '\0';
+       strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate) - 1);

Did you look at the definition and usages of the struct member?
sqlstate is a char[5], which is to be filled with 5-letter SQLSTATE
code not terminated by NUL, which can be shorter if NUL is found
anywhere (I'm not sure there's actually a case of a shorter state
code). If you put NUL to the 5th element of the array, you break the
content. The existing code looks perfect to me.

Sorry, you are right.

-       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.

regards,
Ranier Vilela

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.

/messages/by-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 terminating nul 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, or unsigned char, or pointer to such a type is intended to store character arrays that do not necessarily contain a terminating NUL. 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 function such as strncpy. For example, without the attribute, GCC will issue a warning for the strncpy call below because it may truncate the copy without appending the terminating NUL character. Using the attribute makes it possible to suppress the warning. However, when the array is declared with the attribute the call to strlen is diagnosed because when the array doesn’t contain a NUL-terminated string the call is undefined. To copy, compare, of search non-string character arrays use the memcpy, memcmp, memchr, and other functions that operate on arrays of bytes. In addition, calling strnlen 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

Hi,

On 2021-06-11 19:08:57 -0400, Tom Lane wrote:

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.

Oops, yes.

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.

Hm - the MAXPGPATH stuff is about -Wno-format-truncation though, right?

I now tried building with optimizations and -Wstringop-truncation, and
while it does result in a higher number of warnings, those are all in
ecpg and fixed with one __attribute__((nonstring)).

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. But I think we can just do
something like:

diff --git i/src/interfaces/ecpg/include/sqlca.h w/src/interfaces/ecpg/include/sqlca.h
index c5f107dd33c..d909f5ba2de 100644
--- i/src/interfaces/ecpg/include/sqlca.h
+++ w/src/interfaces/ecpg/include/sqlca.h
@@ -50,7 +50,11 @@ struct sqlca_t
     /* 6: empty                     */
     /* 7: empty                     */

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

struct sqlca_t *ECPGget_sqlca(void);

Not pretty, but I don't immediately see a really better solution?

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

Greetings,

Andres Freund

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

Em sex., 11 de jun. de 2021 às 19:49, Andres Freund <andres@anarazel.de>
escreveu:

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 terminating nul copying 5 bytes
from a string of the same length [-Wstringop-truncation]
565 | strncpy(sqlca->sqlstate, "YE001",
sizeof(sqlca->sqlstate));
|
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

memcpy would not suffer from it?

regards,
Ranier Vilela

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

Em ter., 15 de jun. de 2021 às 15:48, Andres Freund <andres@anarazel.de>
escreveu:

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.

And replacing with snprintf, what do you guys think?

n = snprintf(sqlca->sqlstate, sizeof(sqlca->sqlstate), "%s", sqlstate);
Assert(n >= 0 && n < sizeof(sqlca->sqlstate));

regards,
Ranier Vilela