Обсуждение: unchecked malloc

There are several places in both backend and tools that forget to check the 
return value of malloc(). For example(8.0.1),
   backend/port/dynloader/beos.c/pg_dlopen()   backend/bootstrap/bootstrap.c/AddStr()   port/strdup.c/strdup()
bin/pg_dump/common.c/findParentsByOid()  ...
 

I am thinking we should fix them. Basically we have two ways, one is to 
define a pg_malloc() as psql already did, the other is to fix these places 
one by one. I prefer the first method, since it hides the return value check 
details in the function and less error proning. To report the "out of 
memory" error, we should differenciate if ErrorContext is already set up.

Comments?

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> There are several places in both backend and tools that forget to check the 
> return value of malloc(). For example(8.0.1),

>     backend/port/dynloader/beos.c/pg_dlopen()
Dead port, probably not worth fixing.

>     backend/bootstrap/bootstrap.c/AddStr()
This code will never be run under memory pressure, so althoughit theoretically should be fixed, I'm having a hard time
gettingexcitedabout it.  In any case palloc would be the correct fix.
 

>     port/strdup.c/strdup()
Definitely broken; I just fixed it to conform to the
SUShttp://www.opengroup.org/onlinepubs/007908799/xsh/strdup.html(ofcourse, this code isn't used on any remotely
modernplatform,so it's probably not very relevant...)
 
>     bin/pg_dump/common.c/findParentsByOid()
Probably should be fixed, although if pg_dump runs out of memoryit's just gonna fail anyway.

> To report the "out of memory" error, we should differenciate if
> ErrorContext is already set up.

No, because you're thinking in terms of the backend environment, and
generally in the backend the answer to "when to use malloc directly"
is "never".  The client-side tools are the only place where this is
a serious question, and offhand I'd say "gripe to stderr and exit(1)"
is going to be the right answer in all cases there.
        regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> wrote
>
> No, because you're thinking in terms of the backend environment, and
> generally in the backend the answer to "when to use malloc directly"
> is "never".
>

Well, except before MemoryContext mechanism is set up? For example, the 
functions(e.g., GUC, vfd) used during bootstrap.

So are you suggesting we fix these place by place? This should be ok.

Regards,
Qingqing 

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> wrote
>> No, because you're thinking in terms of the backend environment, and
>> generally in the backend the answer to "when to use malloc directly"
>> is "never".

> Well, except before MemoryContext mechanism is set up? For example, the 
> functions(e.g., GUC, vfd) used during bootstrap.

I think you need to take another look at the startup sequences.  Those
modules are not run before MemoryContextInit.  In any case, the odds
of running out of memory before we get to MemoryContextInit are so small
that I don't have a problem with crashing if it happens.
        regards, tom lane

This dicussion reminds me of a possible memory leak in plpgsql's code. In case you are interested in it;

in pl_comp.c, plpgsql_build_variable takes a pointer to a PLpgSQL_type structure, which is always a malloc'ed instance(since we always use plpgsql_build_datatype function). The switch statement in plpgsql_build_variable function elicits that its reference is only kept in case the type structure represents a PLPGSQL_TTYPE_SCALAR, otherwise it is not kept and needed in case its either PLPGSQL_TTYPE_ROW or PLPGSQL_TTYPE_REC.

So is it intensional or a memory leak?

Thank you

Sibtay Abbas <sibtay@gmail.com> writes:
> in pl_comp.c, plpgsql_build_variable takes a pointer to a PLpgSQL_type
> structure, which is always a malloc'ed instance(since we always use
> plpgsql_build_datatype function).

As of current sources it's palloc'd, and should be released if the
function is ever recompiled, so I see no strong reason to worry.
        regards, tom lane