Problem with setlocale (found in libecpg) [accessing a memory location after freeing it]

On Mon, Sep 24, 2001 at 09:18:42AM +0200, Christof Petig wrote:

well at first I could not believe what I was seeing ...

:-)

Look at the following code (ecpg/lib/execute.c):

const char *locale=setlocale(LC_NUMERIC, NULL);
setlocale(LC_NUMERIC, "C");
[....]
setlocale(LC_NUMERIC, locale);

Well at least on glibc-2.2 it seems that setlocale retuns a pointer to
malloced memory, and frees this pointer on subsequent calls to

Doesn't look that way on my system. The following programs simply dumps core
in free().

#include <locale.h>
#include <stdio.h>

main()
{
const char *locale=setlocale(LC_NUMERIC, NULL);

printf("%c\n", locale);
free(locale);
}

setlocale. This is standard conformant and has good reasons. But used as

You're partially right. Standard says "This string may be allocated in
static storage." So, yes, with your patch we are on the safe side. I just
committed the changes.

Michael

--
Michael Meskes
Michael@Fam-Meskes.De
Go SF 49ers! Go Rhein Fire!
Use Debian GNU/Linux! Use PostgreSQL!

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

Hello,

well at first I could not believe what I was seeing ...

Look at the following code (ecpg/lib/execute.c):

const char *locale=setlocale(LC_NUMERIC, NULL);
setlocale(LC_NUMERIC, "C");
[....]
setlocale(LC_NUMERIC, locale);

Well at least on glibc-2.2 it seems that setlocale retuns a pointer to
malloced memory, and frees this pointer on subsequent calls to
setlocale. This is standard conformant and has good reasons. But used as
above it is lethal (but not lethal enough to be easily recognized). So
the content locale points to is freed by the second call to setlocale.

The remedy is easy (given that _no other_ call to setlocale happens
inbetween ...)

const char *locale=setlocale(LC_NUMERIC, "C");
[...]
setlocale(LC_NUMERIC, locale);

So I would kindly ask you to take a second look at every invokation of
setlocale. And to apply the following patch.

Yours
Christof

[ application/x-gzip is not supported, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Well at least on glibc-2.2 it seems that setlocale retuns a pointer to
malloced memory, and frees this pointer on subsequent calls to
setlocale.
So I would kindly ask you to take a second look at every invokation of
setlocale.

I looked around, and am worried about the behavior of PGLC_current()
in src/backend/utils/adt/pg_locale.c. It doesn't change locale but
does retrieve several successive setlocale() results. Does that work
in glibc?

regards, tom lane

Tom Lane wrote:

Well at least on glibc-2.2 it seems that setlocale retuns a pointer to
malloced memory, and frees this pointer on subsequent calls to
setlocale.
So I would kindly ask you to take a second look at every invokation of
setlocale.

I looked around, and am worried about the behavior of PGLC_current()
in src/backend/utils/adt/pg_locale.c. It doesn't change locale but
does retrieve several successive setlocale() results. Does that work
in glibc?

Well actually I did not check glibc's source code. But I tried to run my
program with efence and it aborted in execute.c

[ locale=setlocale(LC_NUMERIC,NULL);
setlocale(LC_NUMERIC,"C");
...
setlocale(LC_NUMERIC,locale); // access to already freed memory
(locale)
]

So my best guess is that setlocale
- uses a malloced memory for return (which copes best with variable length
strings)
- frees this on a subsequent calls and allocates a new one.

Yes, I'm worried about PGLC_current(), too.
IMHO we should definitely copy the result to a malloced area.
Does the current solution work with static storage (old libcs?)? The last
call would overwrite the first result, wouldn't it?

Christof

On Thu, Sep 27, 2001 at 09:26:12AM +0200, Christof Petig wrote:

Tom Lane wrote:

Well at least on glibc-2.2 it seems that setlocale retuns a pointer to
malloced memory, and frees this pointer on subsequent calls to
setlocale.
So I would kindly ask you to take a second look at every invokation of
setlocale.

I looked around, and am worried about the behavior of PGLC_current()
in src/backend/utils/adt/pg_locale.c. It doesn't change locale but
does retrieve several successive setlocale() results. Does that work
in glibc?

Well actually I did not check glibc's source code. But I tried to run my
program with efence and it aborted in execute.c

I see locale/setlocale.c in glibc (I'm very like that PG hasn't same
coding style as glibc developers:-). You are right with strdup()/free()
in the setlocale().

[ locale=setlocale(LC_NUMERIC,NULL);
setlocale(LC_NUMERIC,"C");
...
setlocale(LC_NUMERIC,locale); // access to already freed memory
(locale)
]

So my best guess is that setlocale
- uses a malloced memory for return (which copes best with variable length
strings)
- frees this on a subsequent calls and allocates a new one.

Yes, I'm worried about PGLC_current(), too.

For example to_char() calls PGLC_localeconv(). In the PGLC_localeconv()
is used:

PGLC_current(&lc);
setlocale(LC_ALL, "");
PGLC_setlocale(&lc); /* <-- access to free memory ? */

I see now it in detail and Christof probably found really pretty bug.
Some users already notice something like:

test=# select to_char(45123.4, 'L99G999D9');
NOTICE: pg_setlocale(): 'LC_MONETARY=pďż˝-@ďż˝' cannot be honored.
to_char
-------------
K� 45�123,4
(1 row)

(I use Czech locales)

We don't see this bug often, because PGLC_localeconv() result is cached
and pg_setlocale is called only once.

It must be fixed for 7.2. May be allocate it in PG, because we need
keep data in PG_LocaleCategories independent on glibc's strdup/free.
May be:

PGLC_current(PG_LocaleCategories * lc)
{
lc->lang = getenv("LANG");

PGLC_free_caltegories(lc);

lc->lc_ctype = pstrdup(setlocale(LC_CTYPE, NULL));

... etc.
}

void
PGLC_free_caltegories(PG_LocaleCategories * lc)
{
if (lc->lc_ctype)
pfree(lc->lc_ctype);

...etc.
}

Comments? I right now work on patch for this.

--
Karel Zak <zakkr@zf.jcu.cz>
http://home.zf.jcu.cz/~zakkr/

C, PostgreSQL, PHP, WWW, http://docs.linux.cz, http://mape.jcu.cz

On Thu, Sep 27, 2001 at 12:08:29AM -0400, Tom Lane wrote:

Well at least on glibc-2.2 it seems that setlocale retuns a pointer to
malloced memory, and frees this pointer on subsequent calls to
setlocale.
So I would kindly ask you to take a second look at every invokation of
setlocale.

I looked around, and am worried about the behavior of PGLC_current()
in src/backend/utils/adt/pg_locale.c. It doesn't change locale but
does retrieve several successive setlocale() results. Does that work
in glibc?

The patch is attached. Now it's independent on glibc's game of setlocale()
results and free/strdup. It works for me...

Thanks to Christof!

Karel

--
Karel Zak <zakkr@zf.jcu.cz>
http://home.zf.jcu.cz/~zakkr/

C, PostgreSQL, PHP, WWW, http://docs.linux.cz, http://mape.jcu.cz

Michael Meskes wrote:

On Thu, Sep 27, 2001 at 12:08:29AM -0400, Tom Lane wrote:

I looked around, and am worried about the behavior of PGLC_current()
in src/backend/utils/adt/pg_locale.c. It doesn't change locale but
does retrieve several successive setlocale() results. Does that work
in glibc?

I haven't experienced any problem so far, but then I wasn't able to
reproduce Christof's either on my glibc2.2 system.

You have to link with efence to see it (see below). (BTW the bug is in
libecpg)

Otherwise the bug is hidden (setting an illegal locale simply does not do
anything if we ignore it's return value (setlocale returns NULL on
error)). Perhaps outputting a notice to the debug stream if setlocale
fails is a good choice (I don't like to raise a SQL error).

Christof

[More detailed: if the former value is freed, the pointer still points to
a valid memory region (without efence), further processing inside ecpg
will reuse that region for just another string (an input variable's value
in SQL notation).
So setting locale '0' or 'ISO' or 'some string' silently fails.]

On Tue, Sep 25, 2001 at 08:15:06PM +0200, Michael Meskes wrote:

Well at least on glibc-2.2 it seems that setlocale retuns a pointer to
malloced memory, and frees this pointer on subsequent calls to

Doesn't look that way on my system. The following programs simply dumps core
in free().

#include <locale.h>
#include <stdio.h>

main()
{
const char *locale=setlocale(LC_NUMERIC, NULL);

printf("%c\n", locale);
free(locale);
}

Because you bad use setlocale().

The setlocale(LC_NUMERIC, NULL) returns actual LC_NUMERIC setting, but
your program hasn't some setting, because you don't call:

setlocale(LC_NUMERIC, "") or setlocale(LC_NUMERIC, "some_locales")

before setlocale(LC_NUMERIC, NULL), try this program:

#include <stdio.h>
#include <locale.h>
#include <stdlib.h>

int
main()
{
char *locale;

/* create array with locales names */
setlocale(LC_NUMERIC, "");

/* returns data from actual setting */
locale = setlocale(LC_NUMERIC, NULL);

printf("%s\n", locale);
free((void *) locale);
exit(1);
}

and don't forget set LC_ALL before program runnig. With default locales "C"
it is same as with NULL.

Previous code:

$ export LC_ALL="cs_CZ"
$ ./loc
cs_CZ
$ export LC_ALL="C"
$ ./loc
C
Segmentation fault <-- in free()

.... and see locale/setlocale.c in glibc sources :-)

Karel

--
Karel Zak <zakkr@zf.jcu.cz>
http://home.zf.jcu.cz/~zakkr/

C, PostgreSQL, PHP, WWW, http://docs.linux.cz, http://mape.jcu.cz

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

On Thu, Sep 27, 2001 at 12:08:29AM -0400, Tom Lane wrote:

Well at least on glibc-2.2 it seems that setlocale retuns a pointer to
malloced memory, and frees this pointer on subsequent calls to
setlocale.
So I would kindly ask you to take a second look at every invokation of
setlocale.

I looked around, and am worried about the behavior of PGLC_current()
in src/backend/utils/adt/pg_locale.c. It doesn't change locale but
does retrieve several successive setlocale() results. Does that work
in glibc?

The patch is attached. Now it's independent on glibc's game of setlocale()
results and free/strdup. It works for me...

Thanks to Christof!

Karel

--
Karel Zak <zakkr@zf.jcu.cz>
http://home.zf.jcu.cz/~zakkr/

C, PostgreSQL, PHP, WWW, http://docs.linux.cz, http://mape.jcu.cz

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Michael Meskes <meskes@postgresql.org> writes:

You're partially right. Standard says "This string may be allocated in
static storage." So, yes, with your patch we are on the safe side. I just
committed the changes.

This patch wasn't right: setlocale(LC_NUMERIC, "C") returns a string
corresponding to the *new* locale setting, not the old one. Therefore,
while the patched code failed to dump core, it also failed to restore
the previous locale setting as intended. I have committed an updated
version.

regards, tom lane