Warning on contrib/tsearch2

Neil Conway <neilc@samurai.com> writes:

In CVS HEAD:
contrib/tsearch2/dict_syn.c:124: warning: 'slen' is used uninitialized
in this function
Induced by the recent pg_verifymbstr() patch.

Seems to be a genuine bug. Fixed, but I see a worse problem with this
code: random backend code should not, not, not be using fopen()
directly. If you lose control to an elog, which is certainly possible
seeing that this loop calls into the utils/mb subsystem, you'll leak
the file descriptor. Use AllocateFile/FreeFile instead of fopen/fclose.

I find the direct use of malloc/realloc/strdup to be poor style as well
--- backend code that is not using palloc needs to have *very* good
reason to do so, and I see none here.

I'm halfway tempted to change postgres.h to #define these functions to
yield errors, and only allow #undef'ing them in the files that are
supposed to access the C library functions directly.

regards, tom lane

Tom Lane wrote:

Neil Conway <neilc@samurai.com> writes:

In CVS HEAD:
contrib/tsearch2/dict_syn.c:124: warning: 'slen' is used uninitialized
in this function
Induced by the recent pg_verifymbstr() patch.

Seems to be a genuine bug. Fixed, but I see a worse problem with this
code: random backend code should not, not, not be using fopen()
directly. If you lose control to an elog, which is certainly possible
seeing that this loop calls into the utils/mb subsystem, you'll leak
the file descriptor. Use AllocateFile/FreeFile instead of fopen/fclose.

Does that apply to things like plperlu?

I find the direct use of malloc/realloc/strdup to be poor style as well
--- backend code that is not using palloc needs to have *very* good
reason to do so, and I see none here.

I'm halfway tempted to change postgres.h to #define these functions to
yield errors, and only allow #undef'ing them in the files that are
supposed to access the C library functions directly.

not a bad idea :-)

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

... random backend code should not, not, not be using fopen()
directly. If you lose control to an elog, which is certainly possible
seeing that this loop calls into the utils/mb subsystem, you'll leak
the file descriptor. Use AllocateFile/FreeFile instead of fopen/fclose.

Does that apply to things like plperlu?

For stuff that executes in the regular backend environment, yes.
Anyplace you are calling code that could potentially throw an elog(),
you'd better play by the rules.

I'm prepared to believe that libperl's error handling and resource
management conventions were designed by someone who knew what they were
doing --- so in code called from libperl, you need to follow the libperl
coding rules, instead. And if you're calling back into the main backend
from a libperl subroutine, you need to provide an impedance match ---
like a subtransaction controlled by a PG_TRY block.

I'm halfway tempted to change postgres.h to #define these functions to
yield errors, and only allow #undef'ing them in the files that are
supposed to access the C library functions directly.

not a bad idea :-)

Not something to try now, but maybe near the beginning of a devel cycle
...

regards, tom lane

code: random backend code should not, not, not be using fopen()
directly. If you lose control to an elog, which is certainly possible
seeing that this loop calls into the utils/mb subsystem, you'll leak
the file descriptor. Use AllocateFile/FreeFile instead of fopen/fclose.

Will soon in tsearch_core patch

I find the direct use of malloc/realloc/strdup to be poor style as well
--- backend code that is not using palloc needs to have *very* good
reason to do so, and I see none here.

Already in tsearch_core patch.

--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/