Обсуждение: Add null termination to string received in parallel apply worker

Hi,

The parallel apply worker didn't add null termination to the string received
from the leader apply worker via the shared memory queue. This action doesn't
bring bugs as it's binary data but violates the rule established in StringInfo,
which guarantees the presence of a terminating '\0' at the end of the string.

Although the original string in leader already includes the null termination,
but we cannot just send all string including the null termination to the
parallel worker, because that would increase the length while at the receiver
there is still no null termination at data[length] which actually the length +
1 position.

And we also cannot directly modify the received string like data[len] = '\0',
because the data still points a shared buffer maintained by shared memory
queue, so we'd better not modify the data outside of the string length.

So, here is patch to use the standard StringInfo API to store the string while
ensuring the addition of null termination. This can also resolve the Assert
issue raised by another patch[1] currently under discussion.

Thanks to Amit for offlist discussion regarding the analysis and
fix.

[1] https://www.postgresql.org/message-id/CAApHDvp6J4Bq9%3Df36-Z3mNWTsmkgGkSkX1Nwut%2BxhSi1aU8zQg%40mail.gmail.com

Best Regards,
Hou Zhijie

Hi Hou-san.

+ /*
+ * Note that the data received via the shared memory queue is not
+ * null-terminated. So we use the StringInfo API to store the
+ * string so as to maintain the convention that StringInfos has a
+ * trailing null.
+ */

"... that StringInfos has a trailing null."

Probably should be either "StringInfo has" or "StringInfos have"

======
Kind Regards.
Peter Smith.
Fujitsu Australia

On Wed, Oct 11, 2023 at 12:18 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> The parallel apply worker didn't add null termination to the string received
> from the leader apply worker via the shared memory queue. This action doesn't
> bring bugs as it's binary data but violates the rule established in StringInfo,
> which guarantees the presence of a terminating '\0' at the end of the string.
>
> Although the original string in leader already includes the null termination,
> but we cannot just send all string including the null termination to the
> parallel worker, because that would increase the length while at the receiver
> there is still no null termination at data[length] which actually the length +
> 1 position.
>
> And we also cannot directly modify the received string like data[len] = '\0',
> because the data still points a shared buffer maintained by shared memory
> queue, so we'd better not modify the data outside of the string length.
>

Yeah, it may not be a good idea to modify the buffer pointing to
shared memory without any lock as we haven't reserved that part of
memory. So, we can't follow the trick used in exec_bind_message() to
maintain the convention that StringInfos have a trailing null. David,
do you see any better way to fix this case?

--
With Regards,
Amit Kapila.

On 2023-Oct-11, Amit Kapila wrote:

> Yeah, it may not be a good idea to modify the buffer pointing to
> shared memory without any lock as we haven't reserved that part of
> memory. So, we can't follow the trick used in exec_bind_message() to
> maintain the convention that StringInfos have a trailing null. David,
> do you see any better way to fix this case?

I was thinking about this when skimming the other StringInfo thread a
couple of days ago.  I wondered if it wouldn't be more convenient to
change the convention that all StringInfos are null-terminated: what is
really the reason to have them all be like that?  We do keep track of
the exact length of the data in it, so strictly speaking we don't need
the assumption.  Maybe there are some routines that are fragile and end
up reading more data thn 'len' if the null terminator is missing; we
could fix those instead.  Right now, it seems we're doing some
pstrdup'ing and unconstification just to be able to install a \0 in the
right place.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Use it up, wear it out, make it do, or do without"

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I was thinking about this when skimming the other StringInfo thread a
> couple of days ago.  I wondered if it wouldn't be more convenient to
> change the convention that all StringInfos are null-terminated: what is
> really the reason to have them all be like that?

It makes sense for StringInfos containing text, not because the
stringinfo.c routines need it but because callers inspecting the
string will very likely do something that expects nul-termination.
When the StringInfo contains binary data, that argument has little
force of course.

I could see extending the convention for caller-supplied buffers
(as is under discussion in the other thread) to say that the caller
needn't provide a nul-terminated buffer if it is confident that
no reader of the StringInfo will need that.  I'd be even more
inclined than before to tie this to a specification that such a
StringInfo is read-only, though.

In any case, this does not immediately let us jump to the conclusion
that it'd be safe to use such a convention in apply workers.  Aren't
the things being passed around here usually text strings?  Do you
really want to promise that no reader is depending on nul-termination?
That doesn't sound safe either for query strings or data input values.

            regards, tom lane

On Thu, 12 Oct 2023 at 05:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > I was thinking about this when skimming the other StringInfo thread a
> > couple of days ago.  I wondered if it wouldn't be more convenient to
> > change the convention that all StringInfos are null-terminated: what is
> > really the reason to have them all be like that?
>
> It makes sense for StringInfos containing text, not because the
> stringinfo.c routines need it but because callers inspecting the
> string will very likely do something that expects nul-termination.
> When the StringInfo contains binary data, that argument has little
> force of course.

I'd like to know why we're even using StringInfo for receiving bytes
pq_* functions.

It does, unfortunately, seem like we're well down that path now and
changing it would be quite a bit of churn, and not just for core :-(
If we had invented a ByteReceiver or something, then StringInfoData
wouldn't need a cursor field (perhaps with the exception of its use in
string_agg_transfn/string_agg_combine, but that could be done some
other way).

It seems like we're trying to enforce rules that are useful for
StringInfo's likely intended original purpose that are often just not
that relevant to the job it's ended up doing in pq_* functions.  It
would be good if we could relax the most-be-NUL-terminated rule.  It
seems to be causing quite a bit of ugliness around the code.  Just
search for "csave". We often use a variable by that name to save the
char where we temporarily put a NUL so we restore it again.  A comment
exec_bind_message() does admit this is "grotty", which I don't
disagree with.

David

On Thursday, October 12, 2023 12:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hi,

>
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > I was thinking about this when skimming the other StringInfo thread a
> > couple of days ago.  I wondered if it wouldn't be more convenient to
> > change the convention that all StringInfos are null-terminated: what
> > is really the reason to have them all be like that?
>
> It makes sense for StringInfos containing text, not because the stringinfo.c
> routines need it but because callers inspecting the string will very likely do
> something that expects nul-termination.
> When the StringInfo contains binary data, that argument has little force of
> course.
>
> I could see extending the convention for caller-supplied buffers (as is under
> discussion in the other thread) to say that the caller needn't provide a
> nul-terminated buffer if it is confident that no reader of the StringInfo will need
> that.  I'd be even more inclined than before to tie this to a specification that
> such a StringInfo is read-only, though.
>
> In any case, this does not immediately let us jump to the conclusion that it'd be
> safe to use such a convention in apply workers.  Aren't the things being passed
> around here usually text strings?

I think the data passed to parallel apply worker is of mixed types. If we see
the data reading logic for it like logicalrep_read_attrs(), it uses both
pq_getmsgint/pq_getmsgbyte/pq_getmsgint(binary) and pq_getmsgstring(text).

> Do you really want to promise that no reader is depending on nul-termination?

I think we could not make an absolute guarantee in this regard, but currently
all the consumer uses pq_getxxx style functions to read the passed data and it
also takes care to read the text stuff(get the length separately e.g.
logicalrep_read_tuple). So it seems OK to release the rule for it.

OTOH, I am not opposed to keeping the rule intact for the apply worker, just to
share the information and to gather opinions from others.

Best Regards,
Hou zj

On Wed, 11 Oct 2023 at 19:54, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
> The parallel apply worker didn't add null termination to the string received
> from the leader apply worker via the shared memory queue. This action doesn't
> bring bugs as it's binary data but violates the rule established in StringInfo,
> which guarantees the presence of a terminating '\0' at the end of the string.

Just for anyone not following the other thread that you linked,  I
just pushed f0efa5aec, and, providing that sticks, I think we can drop
this discussion now.

That commit relaxes the requirement that the StringInfo must be NUL terminated.

David