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

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

[Hao Lee]

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;
}
}


using the string  "this is a*\n*test for*\n*new blank." as the second input
parameter, when dealing with the the last part of stirng, "new blank.", the
code "eol = strchr(text, '\n');" will return NULL and will go into the
"else" part, and the varialbe "eol" will added an integer, which is the
length of "new blank.",  eol will be 0xa, text is set to 0xa too. this
address will is a system used address, then corruption. I think some
boundary check will be added as following.

do_text_output_multiline(TupOutputState *tstate, char *text)
{
Datumvalues[1];
boolisnull[1] = {false};

while (*text *&& *text) //*if the text is NULL the return, do nothing.*
{
char   *eol;
intlen;

eol = strchr(text, '\n');
if (eol)
{
len = eol - text;

eol++;
}
else
{
len = strlen(text);
   * if (eol) //to check whether the eol is NULL or not.*
eol += len;
}

values[0] = PointerGetDatum(cstring_to_text_with_len(text, len));
do_tup_output(tstate, values, isnull);
pfree(DatumGetPointer(values[0]));
text = eol;
}
}

Best Regards.

RingsC.