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

Hi,

On 2021-06-11 19:08:57 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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