Re: pg_dump vs malloc

I came across a situation today with a pretty bad crash of pg_dump,
due to not checking the return code from malloc(). When looking
through the code, it seems there are a *lot* of places in pg_dump that
doesn't check the malloc return code.

But we do have a pg_malloc() function in there - but from what I can
tell it's only used very sparsely?

Shouldn't we be using that one more or less everywhere, or even #define it?

Or am I missing something in the code here?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> I came across a situation today with a pretty bad crash of pg_dump,
> due to not checking the return code from malloc(). When looking
> through the code, it seems there are a *lot* of places in pg_dump that
> doesn't check the malloc return code.

> But we do have a pg_malloc() function in there - but from what I can
> tell it's only used very sparsely?

> Shouldn't we be using that one more or less everywhere

Yup. Have at it.

regards, tom lane

On Fri, Jun 10, 2011 at 21:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> I came across a situation today with a pretty bad crash of pg_dump,
>> due to not checking the return code from malloc(). When looking
>> through the code, it seems there are a *lot* of places in pg_dump that
>> doesn't check the malloc return code.
>
>> But we do have a pg_malloc() function in there - but from what I can
>> tell it's only used very sparsely?
>
>> Shouldn't we be using that one more or less everywhere
>
> Yup.  Have at it.

Something along the line of this?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Something along the line of this?

I think this is a seriously, seriously bad idea:

> +#define strdup(x) pg_strdup(x)
> +#define malloc(x) pg_malloc(x)
> +#define calloc(x,y) pg_calloc(x, y)
> +#define realloc(x,y) pg_realloc(x, y)

as it will render the code unreadable to people expecting the normal
behavior of these fundamental functions; not to mention break any
call sites that have some other means of dealing with an alloc failure
besides going belly-up. Please take the trouble to do
s/malloc/pg_malloc/g and so on, instead.

regards, tom lane

On 22 June 2011 16:25, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Something along the line of this?

IMHO the redefinition of malloc() looks a bit hairy...can't you just
make the callers use the functions directly?

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Jun 22, 2011 at 17:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Something along the line of this?
>
> I think this is a seriously, seriously bad idea:
>
>> +#define strdup(x) pg_strdup(x)
>> +#define malloc(x) pg_malloc(x)
>> +#define calloc(x,y) pg_calloc(x, y)
>> +#define realloc(x,y) pg_realloc(x, y)
>
> as it will render the code unreadable to people expecting the normal
> behavior of these fundamental functions; not to mention break any
> call sites that have some other means of dealing with an alloc failure
> besides going belly-up.  Please take the trouble to do
> s/malloc/pg_malloc/g and so on, instead.

Ok, I'll try that approach. This seemed like a "nicer" approach, but I
think once written out, i agree with your arguments :-)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Excerpts from Magnus Hagander's message of mié jun 22 11:25:43 -0400 2011:
> On Fri, Jun 10, 2011 at 21:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Magnus Hagander <magnus(at)hagander(dot)net> writes:
> >> I came across a situation today with a pretty bad crash of pg_dump,
> >> due to not checking the return code from malloc(). When looking
> >> through the code, it seems there are a *lot* of places in pg_dump that
> >> doesn't check the malloc return code.
> >
> >> But we do have a pg_malloc() function in there - but from what I can
> >> tell it's only used very sparsely?
> >
> >> Shouldn't we be using that one more or less everywhere
> >
> > Yup.  Have at it.
>
> Something along the line of this?

Huh, do you really need a new file for the four new functions? What's
wrong with common.c?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Magnus Hagander wrote:
> On Wed, Jun 22, 2011 at 17:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Magnus Hagander <magnus(at)hagander(dot)net> writes:
> >> Something along the line of this?
> >
> > I think this is a seriously, seriously bad idea:
> >
> >> +#define strdup(x) pg_strdup(x)
> >> +#define malloc(x) pg_malloc(x)
> >> +#define calloc(x,y) pg_calloc(x, y)
> >> +#define realloc(x,y) pg_realloc(x, y)
> >
> > as it will render the code unreadable to people expecting the normal
> > behavior of these fundamental functions; not to mention break any
> > call sites that have some other means of dealing with an alloc failure
> > besides going belly-up. ?Please take the trouble to do
> > s/malloc/pg_malloc/g and so on, instead.
>
> Ok, I'll try that approach. This seemed like a "nicer" approach, but I
> think once written out, i agree with your arguments :-)

Where are we on this?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Oct 14, 2011 at 21:11, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Magnus Hagander wrote:
>> On Wed, Jun 22, 2011 at 17:48, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> > Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> >> Something along the line of this?
>> >
>> > I think this is a seriously, seriously bad idea:
>> >
>> >> +#define strdup(x) pg_strdup(x)
>> >> +#define malloc(x) pg_malloc(x)
>> >> +#define calloc(x,y) pg_calloc(x, y)
>> >> +#define realloc(x,y) pg_realloc(x, y)
>> >
>> > as it will render the code unreadable to people expecting the normal
>> > behavior of these fundamental functions; not to mention break any
>> > call sites that have some other means of dealing with an alloc failure
>> > besides going belly-up. ?Please take the trouble to do
>> > s/malloc/pg_malloc/g and so on, instead.
>>
>> Ok, I'll try that approach. This seemed like a "nicer" approach, but I
>> think once written out, i agree with your arguments :-)
>
> Where are we on this?

It's still sitting on my personal TODO list, just not with a really
high priority.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Alvaro Herrera wrote:
> Excerpts from Magnus Hagander's message of mi jun 22 11:25:43 -0400 2011:
> > On Fri, Jun 10, 2011 at 21:07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > >> I came across a situation today with a pretty bad crash of pg_dump,
> > >> due to not checking the return code from malloc(). When looking
> > >> through the code, it seems there are a *lot* of places in pg_dump that
> > >> doesn't check the malloc return code.
> > >
> > >> But we do have a pg_malloc() function in there - but from what I can
> > >> tell it's only used very sparsely?
> > >
> > >> Shouldn't we be using that one more or less everywhere
> > >
> > > Yup. Have at it.
> >
> > Something along the line of this?
>
> Huh, do you really need a new file for the four new functions? What's
> wrong with common.c?

I developed the attached patch to handle this. I moved the catalog code
from common.c into dumpcatalog.c, so there are just memory routines now
in common.c. I created new memory routines in pg_dumpall.c because
there is no AH structure in pg_dumpall.c. I then modified all the calls
to use the new routines, and removed the NULL return checks that were no
longer necessary.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> I developed the attached patch to handle this. I moved the catalog code
> from common.c into dumpcatalog.c, so there are just memory routines now
> in common.c. I created new memory routines in pg_dumpall.c because
> there is no AH structure in pg_dumpall.c. I then modified all the calls
> to use the new routines, and removed the NULL return checks that were no
> longer necessary.

Applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +