\x output blowing up

Started by Martijn van Oosterhoutover 20 years ago6 messageshackers
Jump to latest
#1Martijn van Oosterhout
kleptog@svana.org

Hi,

On CVS tip, whenever I do \x output, it dies with an internal error in
glibc on free. If you run it under valgrind, it complains about these
lines of code:

700 {
701 char *my_cell = pg_local_malloc(cell_w[i] + 1);
702
703 [Inv write 1 byte] strcpy(my_cell, *ptr);
704 if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
705 format_numeric_locale(my_cell);
706 if (opt_border < 2)
707 fprintf(fout, "%s\n", my_cell);
708 else
709 [Inv read 1 byte] fprintf(fout, "%-s%*s |\n", my_cell, dwidth - cell_w[i], "");
710 free(my_cell);
711 }

Now, apart from the fact that the cell width != strlen in multibyte
encodings, there must be something else because this is just "select *
from pg_proc" and there are no multiple characters there AFAIK. I can't
see it though.

Hope this helps,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Show quoted text

Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
tool for doing 5% of the work and then sitting around waiting for someone
else to do the other 95% so you can sue them.

#2Martijn van Oosterhout
kleptog@svana.org
In reply to: Martijn van Oosterhout (#1)
Re: \x output blowing up

On Sat, Sep 24, 2005 at 11:45:08PM +0200, Martijn van Oosterhout wrote:

Hi,

On CVS tip, whenever I do \x output, it dies with an internal error in
glibc on free. If you run it under valgrind, it complains about these
lines of code:

<snip>

Ok, I worked out the direct cause, pg_wcswidth only returns the length
upto the first newline and the line it breaks on is the multiline
definition in "_pg_expandarray".

The quick fix should be to only allocate memory if it's going to call
format_numeric_locale(), since then you know it's a number. It makes
the code slightly more convoluated but it should be slightly more
efficient.

I actually have a working psql that handles and displays newlines
properly, but it's too late for 8.1. It fixes all these issues
properly.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Show quoted text

Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
tool for doing 5% of the work and then sitting around waiting for someone
else to do the other 95% so you can sue them.

#3Bruce Momjian
bruce@momjian.us
In reply to: Martijn van Oosterhout (#2)
Re: \x output blowing up

Well, it seems we are going to have to fix it somehow for 8.1. It is
not crashing here so I can't work up a patch. Can you submit a minimal
fix for 8.1? Thanks.

---------------------------------------------------------------------------

Martijn van Oosterhout wrote:
-- Start of PGP signed section.

On Sat, Sep 24, 2005 at 11:45:08PM +0200, Martijn van Oosterhout wrote:

Hi,

On CVS tip, whenever I do \x output, it dies with an internal error in
glibc on free. If you run it under valgrind, it complains about these
lines of code:

<snip>

Ok, I worked out the direct cause, pg_wcswidth only returns the length
upto the first newline and the line it breaks on is the multiline
definition in "_pg_expandarray".

The quick fix should be to only allocate memory if it's going to call
format_numeric_locale(), since then you know it's a number. It makes
the code slightly more convoluated but it should be slightly more
efficient.

I actually have a working psql that handles and displays newlines
properly, but it's too late for 8.1. It fixes all these issues
properly.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
tool for doing 5% of the work and then sitting around waiting for someone
else to do the other 95% so you can sue them.

-- End of PGP section, PGP failed!

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
#4Martijn van Oosterhout
kleptog@svana.org
In reply to: Bruce Momjian (#3)
Re: \x output blowing up

On Sat, Sep 24, 2005 at 07:18:16PM -0400, Bruce Momjian wrote:

Well, it seems we are going to have to fix it somehow for 8.1. It is
not crashing here so I can't work up a patch. Can you submit a minimal
fix for 8.1? Thanks.

Ah, it would only happen if your encoding was UTF-8 since that's the
only case psql handles differently. I've attached a patch which fixes
it. With a bit more rearrangement you could probably simplify it a bit
but this works.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Show quoted text

Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
tool for doing 5% of the work and then sitting around waiting for someone
else to do the other 95% so you can sue them.

Attachments:

patchtext/plain; charset=us-asciiDownload+16-3
#5Bruce Momjian
bruce@momjian.us
In reply to: Martijn van Oosterhout (#4)
Re: [HACKERS] \x output blowing up

Martijn van Oosterhout wrote:
-- Start of PGP signed section.

On Sat, Sep 24, 2005 at 07:18:16PM -0400, Bruce Momjian wrote:

Well, it seems we are going to have to fix it somehow for 8.1. It is
not crashing here so I can't work up a patch. Can you submit a minimal
fix for 8.1? Thanks.

Ah, it would only happen if your encoding was UTF-8 since that's the
only case psql handles differently. I've attached a patch which fixes
it. With a bit more rearrangement you could probably simplify it a bit
but this works.

Fixed. You were right that the use of cell_w was incorrect for
non-numeric values (UTF8), and in fact was just too fragile to use.

I redesigned format_numeric_locale() to return an allocated result,
which removed this problem and simplified the code too.

Patch attached and applied.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Attachments:

/rtmp/difftext/plainDownload+84-82
#6Martijn van Oosterhout
kleptog@svana.org
In reply to: Bruce Momjian (#5)
Re: [HACKERS] \x output blowing up

On Tue, Sep 27, 2005 at 12:32:14PM -0400, Bruce Momjian wrote:

I redesigned format_numeric_locale() to return an allocated result,
which removed this problem and simplified the code too.

Much better, I had that in my own tree but figured it wasn't "minimal"
enough for this stage of beta. It is, IMHO, the much better solution.

Thanks for the fix.
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Show quoted text

Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
tool for doing 5% of the work and then sitting around waiting for someone
else to do the other 95% so you can sue them.