Re: [HACKERS] pg_dump vs malloc
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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
On Fri, Oct 14, 2011 at 21:11, Bruce Momjian wrote: > Magnus Hagander wrote: >> On Wed, Jun 22, 2011 at 17:48, Tom Lane wrote: >> > Magnus Hagander 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
Magnus Hagander wrote: > On Wed, Jun 22, 2011 at 17:48, Tom Lane wrote: > > Magnus Hagander 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 http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
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 wrote: > > Magnus Hagander 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 The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
On Wed, Jun 22, 2011 at 17:48, Tom Lane wrote: > Magnus Hagander 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
On 22 June 2011 16:25, Magnus Hagander 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
Magnus Hagander 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump vs malloc
On Fri, Jun 10, 2011 at 21:07, Tom Lane wrote:
> Magnus Hagander 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/
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index 8410af1..0737505 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -20,7 +20,7 @@ override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS)
OBJS= pg_backup_archiver.o pg_backup_db.o pg_backup_custom.o \
pg_backup_files.o pg_backup_null.o pg_backup_tar.o \
- pg_backup_directory.o dumputils.o compress_io.o $(WIN32RES)
+ pg_backup_directory.o pg_dump_alloc.o dumputils.o compress_io.o $(WIN32RES)
KEYWRDOBJS = keywords.o kwlookup.o
@@ -35,8 +35,8 @@ pg_dump: pg_dump.o common.o pg_dump_sort.o $(OBJS) $(KEYWRDOBJS) | submake-libpq
pg_restore: pg_restore.o $(OBJS) $(KEYWRDOBJS) | submake-libpq submake-libpgport
$(CC) $(CFLAGS) pg_restore.o $(KEYWRDOBJS) $(OBJS) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
-pg_dumpall: pg_dumpall.o dumputils.o $(KEYWRDOBJS) | submake-libpq submake-libpgport
- $(CC) $(CFLAGS) pg_dumpall.o dumputils.o $(KEYWRDOBJS) $(WIN32RES) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
+pg_dumpall: pg_dumpall.o dumputils.o pg_dump_alloc.o $(KEYWRDOBJS) | submake-libpq submake-libpgport
+ $(CC) $(CFLAGS) pg_dumpall.o dumputils.o pg_dump_alloc.o $(KEYWRDOBJS) $(WIN32RES) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
install: all installdirs
$(INSTALL_PROGRAM) pg_dump$(X) '$(DESTDIR)$(bindir)'/pg_dump$(X)
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index a631f64..b194f24 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -980,57 +980,3 @@ simple_string_list_member(SimpleStringList *list, const char *val)
}
-/*
- * Safer versions of some standard C library functions. If an
- * out-of-memory condition occurs, these functions will bail out
- * safely; therefore, their return value is guaranteed to be non-NULL.
- *
- * XXX need to refactor things so that these can be in a file that can be
- * shared by pg_dumpall and pg_restore as well as pg_dump.
- */
-
-char *
-pg_strdup(const char *string)
-{
- char *tmp;
-
- if (!string)
- exit_horribly(NULL, NULL, "cannot duplicate null pointer\n");
- tmp = strdup(string);
- if (!tmp)
- exit_horribly(NULL, NULL, "out of memory\n");
- return tmp;
-}
-
-void *
-pg_malloc(size_t size)
-{
- void *tmp;
-
- tmp = malloc(size);
- if (!tmp)
- exit_horribly(NULL, NULL, "out of memory\n");
- return tmp;
-}
-
-void *
-pg_calloc(size_t nmemb, size_t size)
-{
- void *tmp;
-
- tmp = calloc(nmemb, size);
- if (!tmp)
- exit_horribly(NULL, NULL, "out of memory\n");
- return tmp;
-}
-
-void *
-pg_realloc(void *ptr, size_t size)
-{
- void *tmp;
-
- tmp = realloc(ptr, size);
- if (!tmp)
- exit_horribly(NULL, NULL, "out of memory\n");
- return tmp;
-}
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 0e1037c..78b5e93 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -735,8 +735,6 @@ ArchiveEntry(Archive *AHX,
TocEntry *newToc;
newToc = (TocEntry *) calloc(1, sizeof(TocEntry));
- if (!newToc)
- die_horribly(AH, modulename, "out of memory\n");
AH->tocCount++;
if (dumpId > AH->maxDumpId)
@@ -1131,8 +1129,6 @@ archprintf(Archive *AH, const char *fmt,...)
free(p);
bSize *= 2;
p = (char *) malloc(bSize);
- if (p == NULL)
- exit_horribly(AH, modulename, "out of memory\n");
va_start(ap, fmt);
cnt = vsnprintf(p, bSize, fmt, ap);
va_end(ap);
@@ -1266,8 +1262,6 @@ ahprintf(ArchiveHandle *AH, const char *fmt,...)
free(p);
bSize *= 2;
p = (char *) malloc(bSize);
- if (p == NULL)
- die_horribly(AH, modulename, "out of memory\n");
va_start(ap, fmt);
cnt = vsnprintf(p, bSize, fmt, ap);
va_end(ap);
@@ -1736,8 +1730,6 @@ ReadStr(ArchiveHandle *AH)
else
{
buf = (char *) malloc(l + 1);
- if (!buf)
- die_horribly(AH, modulename, "out of memory\n");
if ((*AH->ReadBufPtr) (AH, (void *) buf, l) != l)
die_horribly(AH, modulename, "unexpected end of file\n");
@@ -1930,8 +1922,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
#endif
AH = (ArchiveHandle *) calloc(1, sizeof(ArchiveHandle));
- if (!AH)
- die_horribly(AH, modulename, "out of memory\n");
/* AH->debugLevel = 100; */
@@ -1976,8 +1966,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
AH->currWithOids = -1; /* fo
Re: [HACKERS] pg_dump vs malloc
Magnus Hagander 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
