Re: [HACKERS] It's seems that the function "do_text_output_multiline" does not suit for format "line1\nline2\n...lineN".

[Tom Lane]

David Rowley  writes:
> On 20 May 2016 at 19:13, Hao Lee  wrote:
>> Today, I am do some works on adding some customized featues to PostgreSQL 
>> 9.6 beta1. But, when i do some output to psql using the fuction 
>> "do_text_output_multiline" with the string just like mentioned in mail 
>> tilte, such as "this is a\ntest for\nnew blank.". the PostgreSQL may lead to 
>> corruption in this function, and i debugged it that found this function can 
>> not dealt with the boundaries properly. The original function code as :

> Thanks for reporting this. It does seem pretty broken. I guess we've
> only gotten away with this due to EXPLAIN output lines always having a
> \n at the end of them, but we should fix this.

Agreed.

> Your proposed fix looks a little bit confused. You could have just
> removed the eol += len; as testing if (eol) in the else will never be
> true as that else is only being hit because eol is NULL.

I think really the right fix is "eol = text + len" rather than modifying
the loop condition.  Almost certainly, that is what the original coder
intended, but typo'd the statement and nobody ever noticed.

> I shuffled things around in there a bit and came up with the attached fix.

I didn't like this version because it duplicated the string-conversion
code, which admittedly is only one line, but not a very simple line.
I pushed something based on "eol = text + len" instead.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] It's seems that the function "do_text_output_multiline" does not suit for format "line1\nline2\n...lineN".

[David Rowley]

On 20 May 2016 at 19:13, Hao Lee  wrote:
>
> Hi all,
>
> Today, I am do some works on adding some customized featues to PostgreSQL 9.6 
> beta1. But, when i do some output to psql using the fuction 
> "do_text_output_multiline" with the string just like mentioned in mail tilte, 
> such as "this is a\ntest for\nnew blank.". the PostgreSQL may lead to 
> corruption in this function, and i debugged it that found this function can 
> not dealt with the boundaries properly. The original function code as :
>
> do_text_output_multiline(TupOutputState *tstate, char *text)
> {
> Datumvalues[1];
> boolisnull[1] = {false};
>
> while (*text)
> {
> char   *eol;
> intlen;
>
> eol = strchr(text, '\n');
> if (eol)
> {
> len = eol - text;
>
> eol++;
> }
> else
> {
> len = strlen(text);
> eol += len;
> }
>
> values[0] = PointerGetDatum(cstring_to_text_with_len(text, len));
> do_tup_output(tstate, values, isnull);
> pfree(DatumGetPointer(values[0]));
> text = eol;
> }
> }
>

Thanks for reporting this. It does seem pretty broken. I guess we've
only gotten away with this due to EXPLAIN output lines always having a
\n at the end of them, but we should fix this.

Your proposed fix looks a little bit confused. You could have just
removed the eol += len; as testing if (eol) in the else will never be
true as that else is only being hit because eol is NULL.

I shuffled things around in there a bit and came up with the attached fix.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


do_text_output_multiline_fix.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers