\x output blowing up
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.
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.
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 19073On 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
Index: print.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/psql/print.c,v
retrieving revision 1.74
diff -c -r1.74 print.c
*** print.c 24 Sep 2005 17:53:27 -0000 1.74
--- print.c 25 Sep 2005 09:16:01 -0000
***************
*** 697,714 ****
else
fputs(" ", fout);
{
char *my_cell = pg_local_malloc(cell_w[i] + 1);
strcpy(my_cell, *ptr);
! if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
! format_numeric_locale(my_cell);
if (opt_border < 2)
fprintf(fout, "%s\n", my_cell);
else
fprintf(fout, "%-s%*s |\n", my_cell, dwidth - cell_w[i], "");
free(my_cell);
}
}
if (opt_border == 2)
--- 697,726 ----
else
fputs(" ", fout);
+ /* We seperate the cases for numeric_locale because for UTF-8 (and
+ * other) encodings, cell_w <= strlen(*ptr). We can't just use
+ * strlen() in the malloc because there needs to be enough room for
+ * format_numeric_locale() to store its result. */
+
+ if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
char *my_cell = pg_local_malloc(cell_w[i] + 1);
strcpy(my_cell, *ptr);
! format_numeric_locale(my_cell);
if (opt_border < 2)
fprintf(fout, "%s\n", my_cell);
else
fprintf(fout, "%-s%*s |\n", my_cell, dwidth - cell_w[i], "");
free(my_cell);
}
+ else
+ {
+ if (opt_border < 2)
+ fprintf(fout, "%s\n", *ptr);
+ else
+ fprintf(fout, "%-s%*s |\n", *ptr, dwidth - cell_w[i], "");
+ }
}
if (opt_border == 2)
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 19073Attachments:
/rtmp/difftext/plainDownload
Index: src/bin/psql/print.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/print.c,v
retrieving revision 1.75
diff -c -c -r1.75 print.c
*** src/bin/psql/print.c 26 Sep 2005 18:09:57 -0000 1.75
--- src/bin/psql/print.c 27 Sep 2005 16:25:54 -0000
***************
*** 86,107 ****
return strlen(my_str) + additional_numeric_locale_len(my_str);
}
! static void
! format_numeric_locale(char *my_str)
{
int i, j, int_len = integer_digits(my_str), leading_digits;
int groupdigits = atoi(grouping);
! char *new_str;
!
! if (my_str[0] == '-')
! my_str++;
!
! new_str = pg_local_malloc(strlen_with_numeric_locale(my_str) + 1);
leading_digits = (int_len % groupdigits != 0) ?
int_len % groupdigits : groupdigits;
! for (i=0, j=0; ; i++, j++)
{
/* Hit decimal point? */
if (my_str[i] == '.')
--- 86,111 ----
return strlen(my_str) + additional_numeric_locale_len(my_str);
}
! static char *
! format_numeric_locale(const char *my_str)
{
int i, j, int_len = integer_digits(my_str), leading_digits;
int groupdigits = atoi(grouping);
! int new_str_start = 0;
! char *new_str = new_str = pg_local_malloc(
! strlen_with_numeric_locale(my_str) + 1);
leading_digits = (int_len % groupdigits != 0) ?
int_len % groupdigits : groupdigits;
! if (my_str[0] == '-') /* skip over sign, affects grouping calculations */
! {
! new_str[0] = my_str[0];
! my_str++;
! new_str_start = 1;
! }
!
! for (i=0, j=new_str_start; ; i++, j++)
{
/* Hit decimal point? */
if (my_str[i] == '.')
***************
*** 130,137 ****
new_str[j] = my_str[i];
}
! strcpy(my_str, new_str);
! free(new_str);
}
/*************************/
--- 134,140 ----
new_str[j] = my_str[i];
}
! return new_str;
}
/*************************/
***************
*** 185,194 ****
}
if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(strlen_with_numeric_locale(*ptr) + 1);
!
! strcpy(my_cell, *ptr);
! format_numeric_locale(my_cell);
fputs(my_cell, fout);
free(my_cell);
}
--- 188,195 ----
}
if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
!
fputs(my_cell, fout);
free(my_cell);
}
***************
*** 261,270 ****
fputs(opt_fieldsep, fout);
if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(strlen_with_numeric_locale(*ptr) + 1);
!
! strcpy(my_cell, *ptr);
! format_numeric_locale(my_cell);
fputs(my_cell, fout);
free(my_cell);
}
--- 262,269 ----
fputs(opt_fieldsep, fout);
if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
!
fputs(my_cell, fout);
free(my_cell);
}
***************
*** 488,500 ****
{
if (opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(cell_w[i] + 1);
- strcpy(my_cell, *ptr);
- format_numeric_locale(my_cell);
fprintf(fout, "%*s%s", widths[i % col_count] - cell_w[i], "", my_cell);
free(my_cell);
! }
else
fprintf(fout, "%*s%s", widths[i % col_count] - cell_w[i], "", *ptr);
}
--- 487,497 ----
{
if (opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
fprintf(fout, "%*s%s", widths[i % col_count] - cell_w[i], "", my_cell);
free(my_cell);
! }
else
fprintf(fout, "%*s%s", widths[i % col_count] - cell_w[i], "", *ptr);
}
***************
*** 697,714 ****
else
fputs(" ", fout);
{
! char *my_cell = pg_local_malloc(cell_w[i] + 1);
!
! strcpy(my_cell, *ptr);
! if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
! format_numeric_locale(my_cell);
if (opt_border < 2)
fprintf(fout, "%s\n", my_cell);
else
fprintf(fout, "%-s%*s |\n", my_cell, dwidth - cell_w[i], "");
free(my_cell);
}
}
if (opt_border == 2)
--- 694,716 ----
else
fputs(" ", fout);
+ if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
!
if (opt_border < 2)
fprintf(fout, "%s\n", my_cell);
else
fprintf(fout, "%-s%*s |\n", my_cell, dwidth - cell_w[i], "");
free(my_cell);
}
+ else
+ {
+ if (opt_border < 2)
+ fprintf(fout, "%s\n", *ptr);
+ else
+ fprintf(fout, "%-s%*s |\n", *ptr, dwidth - cell_w[i], "");
+ }
}
if (opt_border == 2)
***************
*** 837,846 ****
fputs(" ", fout);
else if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(strlen_with_numeric_locale(*ptr) + 1);
- strcpy(my_cell, *ptr);
- format_numeric_locale(my_cell);
html_escaped_print(my_cell, fout);
free(my_cell);
}
--- 839,846 ----
fputs(" ", fout);
else if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
html_escaped_print(my_cell, fout);
free(my_cell);
}
***************
*** 922,931 ****
fputs(" ", fout);
else if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(strlen_with_numeric_locale(*ptr) + 1);
!
! strcpy(my_cell, *ptr);
! format_numeric_locale(my_cell);
html_escaped_print(my_cell, fout);
free(my_cell);
}
--- 922,929 ----
fputs(" ", fout);
else if (opt_align[i % col_count] == 'r' && opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
!
html_escaped_print(my_cell, fout);
free(my_cell);
}
***************
*** 1064,1073 ****
{
if (opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(strlen_with_numeric_locale(*ptr) + 1);
!
! strcpy(my_cell, *ptr);
! format_numeric_locale(my_cell);
latex_escaped_print(my_cell, fout);
free(my_cell);
}
--- 1062,1069 ----
{
if (opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
!
latex_escaped_print(my_cell, fout);
free(my_cell);
}
***************
*** 1177,1186 ****
{
if (opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(strlen_with_numeric_locale(*ptr) + 1);
- strcpy(my_cell, *ptr);
- format_numeric_locale(my_cell);
latex_escaped_print(my_cell, fout);
free(my_cell);
}
--- 1173,1180 ----
{
if (opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
latex_escaped_print(my_cell, fout);
free(my_cell);
}
***************
*** 1277,1286 ****
{
if (opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(strlen_with_numeric_locale(*ptr) + 1);
!
! strcpy(my_cell, *ptr);
! format_numeric_locale(my_cell);
troff_ms_escaped_print(my_cell, fout);
free(my_cell);
}
--- 1271,1278 ----
{
if (opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
!
troff_ms_escaped_print(my_cell, fout);
free(my_cell);
}
***************
*** 1389,1398 ****
fputc('\t', fout);
if (opt_numeric_locale)
{
! char *my_cell = pg_local_malloc(strlen_with_numeric_locale(*ptr) + 1);
!
! strcpy(my_cell, *ptr);
! format_numeric_locale(my_cell);
troff_ms_escaped_print(my_cell, fout);
free(my_cell);
}
--- 1381,1388 ----
fputc('\t', fout);
if (opt_numeric_locale)
{
! char *my_cell = format_numeric_locale(*ptr);
!
troff_ms_escaped_print(my_cell, fout);
free(my_cell);
}
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.