Re: [HACKERS] Missing checks when malloc returns NULL...

2016-09-01 Thread Michael Paquier 
On Thu, Sep 1, 2016 at 11:16 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane  wrote:
>>> Don't see the point really ... it's just more API churn without any
>>> very compelling reason.
>
>> OK, then no objection to your approach. At least I tried.
>
> OK, pushed my version then.  I think we have now dealt with everything
> mentioned in this thread except for the issues in pg_locale.c.  Are
> you planning to tackle that?

Yes, not for this CF though. So I have switched the CF entry as committed.
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-09-01 Thread Tom Lane 
Michael Paquier  writes:
> On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane  wrote:
>> Don't see the point really ... it's just more API churn without any
>> very compelling reason.

> OK, then no objection to your approach. At least I tried.

OK, pushed my version then.  I think we have now dealt with everything
mentioned in this thread except for the issues in pg_locale.c.  Are
you planning to tackle that?

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] Missing checks when malloc returns NULL...

2016-09-01 Thread Michael Paquier 
On Thu, Sep 1, 2016 at 10:03 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Also, we could take one extra step forward then, and just introduce
>> ShmemAllocExtended that holds two flags as per the attached:
>> - SHMEM_ALLOC_ZERO that zeros all the fields
>> - SHMEM_ALLOC_NO_OOM that does not fail
>
> Don't see the point really ... it's just more API churn without any
> very compelling reason.

OK, then no objection to your approach. At least I tried.
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-09-01 Thread Tom Lane 
Michael Paquier  writes:
> Also, we could take one extra step forward then, and just introduce
> ShmemAllocExtended that holds two flags as per the attached:
> - SHMEM_ALLOC_ZERO that zeros all the fields
> - SHMEM_ALLOC_NO_OOM that does not fail

Don't see the point really ... it's just more API churn without any
very compelling reason.

(It doesn't look like you correctly implemented the case of both
flags being set, either.)

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] Missing checks when malloc returns NULL...

2016-08-31 Thread Michael Paquier 
On Thu, Sep 1, 2016 at 12:14 AM, Tom Lane  wrote:
> I still think it'd be better to fix that as attached, because it
> represents a net reduction not net addition of code, and it provides
> a defense against future repetitions of the same omission.  If only
> 4 out of 11 existing calls were properly checked --- some of them
> adjacent to calls with checks --- that should tell us that we *will*
> have more instances of the same bug if we don't fix it centrally.
>
> I also note that your patch missed checks for two ShmemAlloc calls in
> InitShmemAllocation and ShmemInitStruct.  Admittedly, since those are
> the very first such calls, it's highly unlikely they'd fail; but I think
> this exercise is not about dismissing failures as improbable.  Almost
> all of these failures are improbable, given that we precalculate the
> shmem space requirement.

OK, that looks fine to me after review.

Also, we could take one extra step forward then, and just introduce
ShmemAllocExtended that holds two flags as per the attached:
- SHMEM_ALLOC_ZERO that zeros all the fields
- SHMEM_ALLOC_NO_OOM that does not fail
Or we could just put a call to MemSet directly in ShmemAlloc(), but
I'd rather keep the base routines extensible.

What do you think about the attached? One other possibility would be
to take your patch, but use MemSet unconditionally on it as this
should not cause any overhead.
-- 
Michael


malloc-nulls-v8.patch
Description: invalid/octet-stream

-- 
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] Missing checks when malloc returns NULL...

2016-08-31 Thread Tom Lane 
Michael Paquier  writes:
> Which means that processes have an escape window when initializing
> shared memory by cleaning up the index if an entry cannot be found and
> then cannot be created properly. I don't think that it is a good idea
> to change that by forcing ShmemAlloc to fail. So I would tend to just
> have the patch attached and add those missing NULL-checks on all the
> existing ShmemAlloc() calls.

> Opinions?

I still think it'd be better to fix that as attached, because it
represents a net reduction not net addition of code, and it provides
a defense against future repetitions of the same omission.  If only
4 out of 11 existing calls were properly checked --- some of them
adjacent to calls with checks --- that should tell us that we *will*
have more instances of the same bug if we don't fix it centrally.

I also note that your patch missed checks for two ShmemAlloc calls in
InitShmemAllocation and ShmemInitStruct.  Admittedly, since those are
the very first such calls, it's highly unlikely they'd fail; but I think
this exercise is not about dismissing failures as improbable.  Almost
all of these failures are improbable, given that we precalculate the
shmem space requirement.

regards, tom lane

diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index 1efe020..cc3af2d 100644
*** a/src/backend/storage/ipc/shmem.c
--- b/src/backend/storage/ipc/shmem.c
*** InitShmemAllocation(void)
*** 163,177 
  /*
   * ShmemAlloc -- allocate max-aligned chunk from shared memory
   *
!  * Assumes ShmemLock and ShmemSegHdr are initialized.
   *
!  * Returns: real pointer to memory or NULL if we are out
!  *		of space.  Has to return a real pointer in order
!  *		to be compatible with malloc().
   */
  void *
  ShmemAlloc(Size size)
  {
  	Size		newStart;
  	Size		newFree;
  	void	   *newSpace;
--- 163,194 
  /*
   * ShmemAlloc -- allocate max-aligned chunk from shared memory
   *
!  * Throws error if request cannot be satisfied.
   *
!  * Assumes ShmemLock and ShmemSegHdr are initialized.
   */
  void *
  ShmemAlloc(Size size)
  {
+ 	void	   *newSpace;
+ 
+ 	newSpace = ShmemAllocNoError(size);
+ 	if (!newSpace)
+ 		ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+  errmsg("out of shared memory (%zu bytes requested)",
+ 		size)));
+ 	return newSpace;
+ }
+ 
+ /*
+  * ShmemAllocNoError -- allocate max-aligned chunk from shared memory
+  *
+  * As ShmemAlloc, but returns NULL if out of space, rather than erroring.
+  */
+ void *
+ ShmemAllocNoError(Size size)
+ {
  	Size		newStart;
  	Size		newFree;
  	void	   *newSpace;
*** ShmemAlloc(Size size)
*** 206,216 
  
  	SpinLockRelease(ShmemLock);
  
! 	if (!newSpace)
! 		ereport(WARNING,
! (errcode(ERRCODE_OUT_OF_MEMORY),
!  errmsg("out of shared memory")));
! 
  	Assert(newSpace == (void *) CACHELINEALIGN(newSpace));
  
  	return newSpace;
--- 223,229 
  
  	SpinLockRelease(ShmemLock);
  
! 	/* note this assert is okay with newSpace == NULL */
  	Assert(newSpace == (void *) CACHELINEALIGN(newSpace));
  
  	return newSpace;
*** ShmemInitHash(const char *name, /* table
*** 293,299 
  	 * The shared memory allocator must be specified too.
  	 */
  	infoP->dsize = infoP->max_dsize = hash_select_dirsize(max_size);
! 	infoP->alloc = ShmemAlloc;
  	hash_flags |= HASH_SHARED_MEM | HASH_ALLOC | HASH_DIRSIZE;
  
  	/* look it up in the shmem index */
--- 306,312 
  	 * The shared memory allocator must be specified too.
  	 */
  	infoP->dsize = infoP->max_dsize = hash_select_dirsize(max_size);
! 	infoP->alloc = ShmemAllocNoError;
  	hash_flags |= HASH_SHARED_MEM | HASH_ALLOC | HASH_DIRSIZE;
  
  	/* look it up in the shmem index */
*** ShmemInitStruct(const char *name, Size s
*** 364,375 
  			 */
  			Assert(shmemseghdr->index == NULL);
  			structPtr = ShmemAlloc(size);
- 			if (structPtr == NULL)
- ereport(ERROR,
- 		(errcode(ERRCODE_OUT_OF_MEMORY),
- 		 errmsg("not enough shared memory for data structure"
- " \"%s\" (%zu bytes requested)",
- name, size)));
  			shmemseghdr->index = structPtr;
  			*foundPtr = FALSE;
  		}
--- 377,382 
*** ShmemInitStruct(const char *name, Size s
*** 410,416 
  	else
  	{
  		/* It isn't in the table yet. allocate and initialize it */
! 		structPtr = ShmemAlloc(size);
  		if (structPtr == NULL)
  		{
  			/* out of memory; remove the failed ShmemIndex entry */
--- 417,423 
  	else
  	{
  		/* It isn't in the table yet. allocate and initialize it */
! 		structPtr = ShmemAllocNoError(size);
  		if (structPtr == NULL)
  		{
  			/* out of memory; remove the failed ShmemIndex entry */
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 7cdb355..4064b20 100644
*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
***

Re: [HACKERS] Missing checks when malloc returns NULL...

2016-08-31 Thread Michael Paquier 
On Wed, Aug 31, 2016 at 7:47 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> [ malloc-nulls-v5.patch ]
>
> I've committed some form of all of these changes

Thanks!

> except the one in
> adt/pg_locale.c.  I'm not entirely sure whether we need to do anything
> about that at all, but if we do, this doesn't cut it:
>
>  thousands_sep = db_encoding_strdup(encoding, extlconv->thousands_sep);
>  grouping = strdup(extlconv->grouping);
>
> +if (!grouping)
> +elog(ERROR, "out of memory");
> +
>  #ifdef WIN32
>  /* use monetary to set the ctype */
>  setlocale(LC_CTYPE, locale_monetary);
>
> There are multiple things wrong with that:
>
> 1. The db_encoding_strdup calls can also return NULL on OOM, and aren't
> being checked likewise.  (And there's another plain strdup further down,
> too.)
>
> 2. You can't safely throw an elog right there, because you need to restore
> the backend's prevailing setlocale state first.

Arg I haven't though of this one.

> 3. Also, if you do it like that, you'll leak whatever strings were already
> strdup'd.  (This is a relatively minor problem, but still, if we're
> trying to be 100% correct then we're not there yet.)
>
> Also, now that I'm looking at it, I see there's another pre-existing bug:
>
> 4. An elog exit is possible, due to either OOM or encoding conversion
> failure, inside db_encoding_strdup().  This means we have problems #2 and
> #3 even in the existing code.
>
> Now, I believe that the coding intention here was that assigning NULL
> to the respective fields of CurrentLocaleConv is okay and better than
> failing the operation completely.  One argument against that is that
> it's unclear whether everyplace that uses any of those fields is checking
> for NULL first; and in any case, silently falling back to nonlocalized
> behavior might not be better than reporting OOM.  Still, it's certainly
> better than risking problem #2, which could cause all sorts of subsequent
> malfunctions.
>
> I think that a complete fix for this might go along the lines of
>
> 1. While we are setlocale'd to the nonstandard locales, collect all the
> values by strdup'ing into a local variable of type "struct lconv".
> (We must strdup for the reason noted in the comment, that localeconv's
> output is no longer valid once we do another setlocale.)  Then restore
> the standard locale settings.

The one at the top of the file... That's really platform-dependent.

> 2. Use db_encoding_strdup to replace any strings that need to be
> converted.  (If it throws an elog, we have no damage worse than
> leaking the already strdup'd strings.)
>
> 3. Check for any nulls in the struct; if so, use free_struct_lconv
> to release whatever we did get, and then throw elog("out of memory").
>
> 4. Otherwise, copy the struct to CurrentLocaleConv.
>
> If we were really feeling picky, we could probably put in a PG_TRY
> block around step 2 to release the strdup'd storage after a conversion
> failure.  Not sure if it's worth the trouble.

It doesn't sound that much complicated to do that, I'll see about it,
but I guess that we could just do it in another thread..

> BTW, I marked the commitfest entry closed, but that may have been
> premature --- feel free to reopen it if you submit additional patches
> in this thread.

There are still two things that could be worked I guess:
- plperl and pltcl cleanup, and their abusive use of malloc. I'll
raise a new thread about that after brushing up a bit what I have and
add a new entry in the CF as that's not directly related to this
thread.
- ShmemAlloc and its missing checks. And we can do something here.

I have slept on it, and looked at the numbers. There are 11 calls to
ShmemAlloc in the code, and 4 of them are performing checks. And in
one of them there is this pattern in ShmemInitStruct(), which is also
something ShmemInitHash relies on:

/* It isn't in the table yet. allocate and initialize it */
structPtr = ShmemAlloc(size);
if (structPtr == NULL)
{
/* out of memory; remove the failed ShmemIndex entry */
hash_search(ShmemIndex, name, HASH_REMOVE, NULL);
LWLockRelease(ShmemIndexLock);
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
 errmsg("not enough shared memory for data structure"
" \"%s\" (%zu bytes requested)",
name, size)));
}
Which means that processes have an escape window when initializing
shared memory by cleaning up the index if an entry cannot be found and
then cannot be created properly. I don't think that it is a good idea
to change that by forcing ShmemAlloc to fail. So I would tend to just
have the patch attached and add those missing NULL-checks on all the
existing ShmemAlloc() calls.

Opinions?
-- 
Michael
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index

Re: [HACKERS] Missing checks when malloc returns NULL...

2016-08-31 Thread Michael Paquier 
On Wed, Aug 31, 2016 at 2:15 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> And with an actual patch things are better.
>
> Working through this patch, it suddenly strikes me that we are going
> about fixing the callers of simple_prompt the wrong way.  The existing
> definition with returning a malloc'd string creates a hazard of malloc
> failure, and it *also* creates a hazard of not remembering to free the
> result.

Yes, this cleanup was part of the candidate patch of this thread as well.

> Moreover, there are almost no callers that want a max result
> longer than ~100 bytes.

True, there is basically one such caller, psql, with 4096 bytes.

> Seems like it would be a whole lot easier all
> around to make the caller supply the buffer, ie typical call would be
> roughly
>
> charbuf[100];
>
> simple_prompt("Password: ", buf, sizeof(buf), false);
>
> Callers that want to deal with a malloc'd buffer (all one of them, looks
> like) can do it themselves, for basically only one more line than is
> needed now.

Yes, that's possible as well and I thought about doing so, but I found
the buffer allocated from within simple_prompt clearer when hacking
this part. By the way, I just reviewed 9daec77e that you pushed
instead and that looks fine to me. Thanks!
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-08-30 Thread Tom Lane 
Michael Paquier  writes:
> [ malloc-nulls-v5.patch ]

I've committed some form of all of these changes except the one in
adt/pg_locale.c.  I'm not entirely sure whether we need to do anything
about that at all, but if we do, this doesn't cut it:

 thousands_sep = db_encoding_strdup(encoding, extlconv->thousands_sep);
 grouping = strdup(extlconv->grouping);
 
+if (!grouping)
+elog(ERROR, "out of memory");
+
 #ifdef WIN32
 /* use monetary to set the ctype */
 setlocale(LC_CTYPE, locale_monetary);

There are multiple things wrong with that:

1. The db_encoding_strdup calls can also return NULL on OOM, and aren't
being checked likewise.  (And there's another plain strdup further down,
too.)

2. You can't safely throw an elog right there, because you need to restore
the backend's prevailing setlocale state first.

3. Also, if you do it like that, you'll leak whatever strings were already
strdup'd.  (This is a relatively minor problem, but still, if we're
trying to be 100% correct then we're not there yet.)

Also, now that I'm looking at it, I see there's another pre-existing bug:

4. An elog exit is possible, due to either OOM or encoding conversion
failure, inside db_encoding_strdup().  This means we have problems #2 and
#3 even in the existing code.

Now, I believe that the coding intention here was that assigning NULL
to the respective fields of CurrentLocaleConv is okay and better than
failing the operation completely.  One argument against that is that
it's unclear whether everyplace that uses any of those fields is checking
for NULL first; and in any case, silently falling back to nonlocalized
behavior might not be better than reporting OOM.  Still, it's certainly
better than risking problem #2, which could cause all sorts of subsequent
malfunctions.

I think that a complete fix for this might go along the lines of

1. While we are setlocale'd to the nonstandard locales, collect all the
values by strdup'ing into a local variable of type "struct lconv".
(We must strdup for the reason noted in the comment, that localeconv's
output is no longer valid once we do another setlocale.)  Then restore
the standard locale settings.

2. Use db_encoding_strdup to replace any strings that need to be
converted.  (If it throws an elog, we have no damage worse than
leaking the already strdup'd strings.)

3. Check for any nulls in the struct; if so, use free_struct_lconv
to release whatever we did get, and then throw elog("out of memory").

4. Otherwise, copy the struct to CurrentLocaleConv.

If we were really feeling picky, we could probably put in a PG_TRY
block around step 2 to release the strdup'd storage after a conversion
failure.  Not sure if it's worth the trouble.

BTW, I marked the commitfest entry closed, but that may have been
premature --- feel free to reopen it if you submit additional patches
in this thread.

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] Missing checks when malloc returns NULL...

2016-08-30 Thread Tom Lane 
Michael Paquier  writes:
> And with an actual patch things are better.

Working through this patch, it suddenly strikes me that we are going
about fixing the callers of simple_prompt the wrong way.  The existing
definition with returning a malloc'd string creates a hazard of malloc
failure, and it *also* creates a hazard of not remembering to free the
result.  Moreover, there are almost no callers that want a max result
longer than ~100 bytes.  Seems like it would be a whole lot easier all
around to make the caller supply the buffer, ie typical call would be
roughly

charbuf[100];

simple_prompt("Password: ", buf, sizeof(buf), false);

Callers that want to deal with a malloc'd buffer (all one of them, looks
like) can do it themselves, for basically only one more line than is
needed now.

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] Missing checks when malloc returns NULL...

2016-08-30 Thread Tom Lane 
Aleksander Alekseev  writes:
> I suggest to keep ShmemAlloc as is for backward compatibility and
> introduce a new procedure ShmemAllocSafe.

I think that's about the worst of all possible worlds, as it guarantees
having to touch most call sites.  If there were more than one known caller
that really wanted the return-NULL behavior, exact backwards compatibility
might carry the day; but as things stand I think the odds are that most
call sites need an error check and probably have not got one.  I'd rather
err in favor of "safe by default" than "backwards compatible".

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] Missing checks when malloc returns NULL...

2016-08-30 Thread Tom Lane 
Michael Paquier  writes:
> On Tue, Aug 30, 2016 at 10:18 PM, Tom Lane  wrote:
>> I think what we ought to do is make ShmemAlloc act like palloc
>> (ie throw error not return NULL), and remove the duplicated error
>> checks.

> The only reason why I did not propose that for ShmemAlloc is because
> of extensions potentially using this routine and having some special
> handling when it returns NULL. And changing it to behave like palloc
> would break such extensions.

The evidence from the callers in core suggests that this change
would be much more likely to fix extensions than break them,
ie it's more likely that they are missing error checks than that
they have something useful to do if the alloc fails.

An extension that actually does need that could do something like

#if CATALOG_VERSION_NO < whatever-v10-is
#define ShmemAllocNoError(x) ShmemAlloc(x)
#endif

...

ptr = ShmemAllocNoError(size);
if (ptr == NULL)  // same as before from here on


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] Missing checks when malloc returns NULL...

2016-08-30 Thread Aleksander Alekseev 
> > I think what we ought to do is make ShmemAlloc act like palloc
> > (ie throw error not return NULL), and remove the duplicated error
> > checks.  For the one caller that that would be bad for, we could
> > invent something like ShmemAllocNoError, or ShmemAllocExtended with
> > a no_error flag, or whatever other new API suits your fancy.  But
> > as-is, it's just inviting more errors-of-omission like the large
> > number that already exist.  I think people are conditioned by palloc
> > to expect such functions to throw error.
> 
> The only reason why I did not propose that for ShmemAlloc is because
> of extensions potentially using this routine and having some special
> handling when it returns NULL. And changing it to behave like palloc
> would break such extensions.

I suggest to keep ShmemAlloc as is for backward compatibility and
introduce a new procedure ShmemAllocSafe. Also we could add a scary
comment (and also a warning log message?) that ShmemAlloc is deprecated
and possibly will be removed in later releases.

-- 
Best regards,
Aleksander Alekseev


-- 
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] Missing checks when malloc returns NULL...

2016-08-30 Thread Michael Paquier 
On Tue, Aug 30, 2016 at 10:18 PM, Tom Lane  wrote:
> I think what we ought to do is make ShmemAlloc act like palloc
> (ie throw error not return NULL), and remove the duplicated error
> checks.  For the one caller that that would be bad for, we could
> invent something like ShmemAllocNoError, or ShmemAllocExtended with
> a no_error flag, or whatever other new API suits your fancy.  But
> as-is, it's just inviting more errors-of-omission like the large
> number that already exist.  I think people are conditioned by palloc
> to expect such functions to throw error.

The only reason why I did not propose that for ShmemAlloc is because
of extensions potentially using this routine and having some special
handling when it returns NULL. And changing it to behave like palloc
would break such extensions.
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-08-30 Thread Tom Lane 
Michael Paquier  writes:
>> I've just realized that there is also malloc-compatible ShmemAlloc().
>> Apparently it's return value sometimes is not properly checked too. See
>> attachment.

> The funny part here is that ProcGlobal->allProcs is actually handled,
> but not the two others. Well yes, you are right, we really need to
> fail on FATAL for all of them if ShmemAlloc returns NULL as they
> involve the shmem initialization at postmaster startup.

A quick scan says that most callers are failing to check at all,
several more just have duplicate check-and-immediately-elog code,
and only one has got anything useful to do besides throwing error.

I think what we ought to do is make ShmemAlloc act like palloc
(ie throw error not return NULL), and remove the duplicated error
checks.  For the one caller that that would be bad for, we could
invent something like ShmemAllocNoError, or ShmemAllocExtended with
a no_error flag, or whatever other new API suits your fancy.  But
as-is, it's just inviting more errors-of-omission like the large
number that already exist.  I think people are conditioned by palloc
to expect such functions to throw error.

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] Missing checks when malloc returns NULL...

2016-08-30 Thread Aleksander Alekseev 
> >> And with an actual patch things are better.
> >
> > Currently I can't think of any further improvements. I even would dare
> > to say that patch is Ready for Committer.
> 
> Thanks for the fruitful input by the way! You spotted many things.

Thank _you_ for paying attention for such issues in PostgreSQL code!

-- 
Best regards,
Aleksander Alekseev


-- 
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] Missing checks when malloc returns NULL...

2016-08-30 Thread Michael Paquier 
On Tue, Aug 30, 2016 at 5:08 PM, Aleksander Alekseev
 wrote:
>> > The funny part here is that ProcGlobal->allProcs is actually handled,
>> > but not the two others. Well yes, you are right, we really need to
>> > fail on FATAL for all of them if ShmemAlloc returns NULL as they
>> > involve the shmem initialization at postmaster startup.
>>
>> And with an actual patch things are better.
>
> Currently I can't think of any further improvements. I even would dare
> to say that patch is Ready for Committer.

Thanks for the fruitful input by the way! You spotted many things.
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-08-30 Thread Aleksander Alekseev 
> > The funny part here is that ProcGlobal->allProcs is actually handled,
> > but not the two others. Well yes, you are right, we really need to
> > fail on FATAL for all of them if ShmemAlloc returns NULL as they
> > involve the shmem initialization at postmaster startup.
> 
> And with an actual patch things are better.

Currently I can't think of any further improvements. I even would dare
to say that patch is Ready for Committer.

-- 
Best regards,
Aleksander Alekseev


-- 
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] Missing checks when malloc returns NULL...

2016-08-30 Thread Michael Paquier 
On Tue, Aug 30, 2016 at 2:57 PM, Michael Paquier
 wrote:
> The funny part here is that ProcGlobal->allProcs is actually handled,
> but not the two others. Well yes, you are right, we really need to
> fail on FATAL for all of them if ShmemAlloc returns NULL as they
> involve the shmem initialization at postmaster startup.

And with an actual patch things are better.
-- 
Michael
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index e5eeec2..5dd0046 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -306,6 +306,11 @@ sql_conn(struct options * my_opts)
 		{
 			PQfinish(conn);
 			password = simple_prompt("Password: ", 100, false);
+			if (!password)
+			{
+fprintf(stderr, "%s: out of memory\n", "oid2name");
+exit(1);
+			}
 			new_pass = true;
 		}
 	} while (new_pass);
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 5eac2b1..e4136f9 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -632,7 +632,7 @@ main(int argc, char **argv)
 }
 break;
 			case 't':			/* Trigger file */
-triggerPath = strdup(optarg);
+triggerPath = pg_strdup(optarg);
 break;
 			case 'w':			/* Max wait time */
 maxwaittime = atoi(optarg);
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 769c805..2b0faf0 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -71,7 +71,14 @@ vacuumlo(const char *database, const struct _param * param)
 
 	/* Note: password can be carried over from a previous call */
 	if (param->pg_prompt == TRI_YES && password == NULL)
+	{
 		password = simple_prompt("Password: ", 100, false);
+		if (!password)
+		{
+			fprintf(stderr, "out of memory\n");
+			return -1;
+		}
+	}
 
 	/*
 	 * Start the connection.  Loop until we have a password if requested by
@@ -115,6 +122,11 @@ vacuumlo(const char *database, const struct _param * param)
 		{
 			PQfinish(conn);
 			password = simple_prompt("Password: ", 100, false);
+			if (!password)
+			{
+fprintf(stderr, "out of memory\n");
+return -1;
+			}
 			new_pass = true;
 		}
 	} while (new_pass);
@@ -514,7 +526,7 @@ main(int argc, char **argv)
 }
 break;
 			case 'U':
-param.pg_user = strdup(optarg);
+param.pg_user = pg_strdup(optarg);
 break;
 			case 'w':
 param.pg_prompt = TRI_NO;
@@ -529,10 +541,10 @@ main(int argc, char **argv)
 	fprintf(stderr, "%s: invalid port number: %s\n", progname, optarg);
 	exit(1);
 }
-param.pg_port = strdup(optarg);
+param.pg_port = pg_strdup(optarg);
 break;
 			case 'h':
-param.pg_host = strdup(optarg);
+param.pg_host = pg_strdup(optarg);
 break;
 		}
 	}
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index bbae584..d1a26ef 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -212,6 +212,10 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
 
 		/* Initialize LWLocks */
 		shared->buffer_locks = (LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
+		if (!shared->buffer_locks)
+			ereport(FATAL,
+	(errcode(ERRCODE_OUT_OF_MEMORY),
+	 errmsg("out of shared memory")));
 
 		Assert(strlen(name) + 1 < SLRU_MAX_NAME_LENGTH);
 		strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 8feeae0..5fd6290 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -569,8 +569,15 @@ boot_openrel(char *relname)
 			++i;
 		heap_endscan(scan);
 		app = Typ = ALLOC(struct typmap *, i + 1);
+		if (app == NULL)
+			elog(ERROR, "out of memory");
 		while (i-- > 0)
-			*app++ = ALLOC(struct typmap, 1);
+		{
+			*app = ALLOC(struct typmap, 1);
+			if (*app == NULL)
+elog(ERROR, "out of memory");
+			app++;
+		}
 		*app = NULL;
 		scan = heap_beginscan_catalog(rel, 0, NULL);
 		app = Typ;
@@ -894,8 +901,15 @@ gettype(char *type)
 			++i;
 		heap_endscan(scan);
 		app = Typ = ALLOC(struct typmap *, i + 1);
+		if (app == NULL)
+			elog(ERROR, "out of memory");
 		while (i-- > 0)
-			*app++ = ALLOC(struct typmap, 1);
+		{
+			*app = ALLOC(struct typmap, 1);
+			if (*app == NULL)
+elog(ERROR, "out of memory");
+			app++;
+		}
 		*app = NULL;
 		scan = heap_beginscan_catalog(rel, 0, NULL);
 		app = Typ;
diff --git a/src/backend/port/dynloader/darwin.c b/src/backend/port/dynloader/darwin.c
index ccd92c3..a83c614 100644
--- a/src/backend/port/dynloader/darwin.c
+++ b/src/backend/port/dynloader/darwin.c
@@ -78,6 +78,9 @@ pg_dlsym(void *handle, char *funcname)
 	NSSymbol symbol;
 	char	   *symname = (char *) malloc(strlen(funcname) + 2);
 
+	if (!symname)
+		return NULL;
+
 	sprintf(symname, "_%s", funcname);
 	if (NSIsSymbolNameDefined(symname))
 	{
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c

Re: [HACKERS] Missing checks when malloc returns NULL...

2016-08-30 Thread Michael Paquier 
On Mon, Aug 29, 2016 at 11:26 PM, Tom Lane  wrote:
> Aleksander Alekseev  writes:
>>> if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
>>> + {
>>> +free(prodesc);
>
>> I think that prodesc->user_proname and prodesc->internal_proname should
>> also be freed if they are not NULL's.
>
> Hmm, this is kind of putting lipstick on a pig, isn't it?  That code
> is still prone to leakage further down, because it calls stuff like
> SearchSysCache which is entirely capable of throwing elog(ERROR).
>
> If we're going to touch compile_pltcl_function at all, I'd vote for
>
> (1) changing these malloc calls to MemoryContextAlloc(TopMemoryContext,...
> (2) putting the cleanup into a PG_CATCH block, and removing all the
> retail free() calls that are there now.

We've done similar handling lately with for example 8c75ad43 for
plpython, and this has finished by using TopMemoryContext, so that's
the way to do. By the way plperl needs the same cleanup, and by
looking at the archives guess who did exactly that with a set of
patches not long ago:
https://www.postgresql.org/message-id/cab7npqrxvq+q66ufzd9wa5uaftyn4wauadbjxkfrync96kf...@mail.gmail.com
But I did not get much feedback about those patches :)

So for now I have removed this part from the patch of this thread.
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-08-29 Thread Michael Paquier 
On Mon, Aug 29, 2016 at 11:13 PM, Aleksander Alekseev
 wrote:
>> Hello, Michael
>>
>> > I don't know how you did it, but this email has visibly broken the
>> > original thread. Did you change the topic name?
>>
>> I'm very sorry for this. I had no local copy of this thread. So I wrote a
>> new email with the same subject hoping it will be OK. Apparently right
>> In-Reply-To header is also required.
>>
>> >   if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
>> > + {
>> > +free(prodesc);
>>
>> I think that prodesc->user_proname and prodesc->internal_proname should
>> also be freed if they are not NULL's.
>>
>> > By the way maybe someone knows other procedures besides malloc, realloc
>> > and strdup that require special attention?
>>
>> I recalled that there is also calloc(). I've found four places that use
>> calloc() and look suspicious to me (see attachment). What do you think -
>> are these bugs or not?

./src/backend/storage/buffer/localbuf.c:LocalBufferBlockPointers =
(Block *) calloc(nbufs, sizeof(Block));
./src/interfaces/libpq/fe-print.c-  fprintf(stderr,
libpq_gettext("out of memory\n"));
Here it does not matter the process is taken down with FATAL or
abort() immediately.

./src/backend/bootstrap/bootstrap.c:app = Typ =
ALLOC(struct typmap *, i + 1);
But here it does actually matter.

> I've just realized that there is also malloc-compatible ShmemAlloc().
> Apparently it's return value sometimes is not properly checked too. See
> attachment.

./src/backend/storage/lmgr/proc.c:  pgxacts = (PGXACT *)
ShmemAlloc(TotalProcs * sizeof(PGXACT));
./src/backend/storage/lmgr/proc.c:  ProcStructLock = (slock_t *)
ShmemAlloc(sizeof(slock_t));
./src/backend/storage/lmgr/lwlock.c:ptr = (char *)
ShmemAlloc(spaceLocks);
./src/backend/storage/ipc/shmem.c:  ShmemAlloc(sizeof(*ShmemVariableCache));
./src/backend/access/transam/slru.c:shared->buffer_locks =
(LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
./src/backend/postmaster/postmaster.c:  ShmemBackendArray = (Backend
*) ShmemAlloc(size);

The funny part here is that ProcGlobal->allProcs is actually handled,
but not the two others. Well yes, you are right, we really need to
fail on FATAL for all of them if ShmemAlloc returns NULL as they
involve the shmem initialization at postmaster startup.
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-08-29 Thread Tom Lane 
Aleksander Alekseev  writes:
>> if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
>> + {
>> +free(prodesc);

> I think that prodesc->user_proname and prodesc->internal_proname should
> also be freed if they are not NULL's.

Hmm, this is kind of putting lipstick on a pig, isn't it?  That code
is still prone to leakage further down, because it calls stuff like
SearchSysCache which is entirely capable of throwing elog(ERROR).

If we're going to touch compile_pltcl_function at all, I'd vote for

(1) changing these malloc calls to MemoryContextAlloc(TopMemoryContext,...

(2) putting the cleanup into a PG_CATCH block, and removing all the
retail free() calls that are there now.

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] Missing checks when malloc returns NULL...

2016-08-29 Thread Aleksander Alekseev 
> Hello, Michael
> 
> > I don't know how you did it, but this email has visibly broken the
> > original thread. Did you change the topic name?
> 
> I'm very sorry for this. I had no local copy of this thread. So I wrote a
> new email with the same subject hoping it will be OK. Apparently right
> In-Reply-To header is also required. 
> 
> >   if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
> > + {
> > +free(prodesc);
> 
> I think that prodesc->user_proname and prodesc->internal_proname should
> also be freed if they are not NULL's.
> 
> > By the way maybe someone knows other procedures besides malloc, realloc
> > and strdup that require special attention?
> 
> I recalled that there is also calloc(). I've found four places that use
> calloc() and look suspicious to me (see attachment). What do you think -
> are these bugs or not?

I've just realized that there is also malloc-compatible ShmemAlloc().
Apparently it's return value sometimes is not properly checked too. See
attachment.

-- 
Best regards,
Aleksander Alekseev
./src/backend/access/transam/slru.c-/* Initialize LWLocks */
./src/backend/access/transam/slru.c:shared->buffer_locks = 
(LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
./src/backend/access/transam/slru.c-
./src/backend/access/transam/slru.c-Assert(strlen(name) + 1 < 
SLRU_MAX_NAME_LENGTH);
./src/backend/access/transam/slru.c-
strlcpy(shared->lwlock_tranche_name, name, SLRU_MAX_NAME_LENGTH);
./src/backend/access/transam/slru.c-shared->lwlock_tranche_id = 
tranche_id;
./src/backend/access/transam/slru.c-shared->lwlock_tranche.name = 
shared->lwlock_tranche_name;

./src/backend/postmaster/postmaster.c-void
./src/backend/postmaster/postmaster.c-ShmemBackendArrayAllocation(void)
./src/backend/postmaster/postmaster.c-{
./src/backend/postmaster/postmaster.c-  Sizesize = 
ShmemBackendArraySize();
./src/backend/postmaster/postmaster.c-
./src/backend/postmaster/postmaster.c:  ShmemBackendArray = (Backend *) 
ShmemAlloc(size);
./src/backend/postmaster/postmaster.c-  /* Mark all slots as empty */
./src/backend/postmaster/postmaster.c-  memset(ShmemBackendArray, 0, size);
./src/backend/postmaster/postmaster.c-}

./src/backend/storage/ipc/shmem.c-  ShmemVariableCache = (VariableCache)
./src/backend/storage/ipc/shmem.c:  ShmemAlloc(sizeof(*ShmemVariableCache));
./src/backend/storage/ipc/shmem.c-  memset(ShmemVariableCache, 0, 
sizeof(*ShmemVariableCache));
./src/backend/storage/ipc/shmem.c-}

./src/backend/storage/lmgr/lwlock.c-/* Allocate space */
./src/backend/storage/lmgr/lwlock.c:ptr = (char *) 
ShmemAlloc(spaceLocks);
./src/backend/storage/lmgr/lwlock.c-
./src/backend/storage/lmgr/lwlock.c-/* Leave room for dynamic 
allocation of tranches */
./src/backend/storage/lmgr/lwlock.c-ptr += sizeof(int);

./src/backend/storage/lmgr/proc.c:  pgxacts = (PGXACT *) ShmemAlloc(TotalProcs 
* sizeof(PGXACT));
./src/backend/storage/lmgr/proc.c-  MemSet(pgxacts, 0, TotalProcs * 
sizeof(PGXACT));

./src/backend/storage/lmgr/proc.c-  /* Create ProcStructLock spinlock, too */
./src/backend/storage/lmgr/proc.c:  ProcStructLock = (slock_t *) 
ShmemAlloc(sizeof(slock_t));
./src/backend/storage/lmgr/proc.c-  SpinLockInit(ProcStructLock);


-- 
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] Missing checks when malloc returns NULL...

2016-08-29 Thread Aleksander Alekseev 
Hello, Michael

> I don't know how you did it, but this email has visibly broken the
> original thread. Did you change the topic name?

I'm very sorry for this. I had no local copy of this thread. So I wrote a
new email with the same subject hoping it will be OK. Apparently right
In-Reply-To header is also required. 

>   if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL)
> + {
> +free(prodesc);

I think that prodesc->user_proname and prodesc->internal_proname should
also be freed if they are not NULL's.

> By the way maybe someone knows other procedures besides malloc, realloc
> and strdup that require special attention?

I recalled that there is also calloc(). I've found four places that use
calloc() and look suspicious to me (see attachment). What do you think -
are these bugs or not?

-- 
Best regards,
Aleksander Alekseev
Looks like resource leak:

./src/backend/storage/buffer/localbuf.c:LocalBufferDescriptors = 
(BufferDesc *) calloc(nbufs, sizeof(BufferDesc));
./src/backend/storage/buffer/localbuf.c:LocalBufferBlockPointers = (Block 
*) calloc(nbufs, sizeof(Block));
./src/backend/storage/buffer/localbuf.c:LocalRefCount = (int32 *) 
calloc(nbufs, sizeof(int32));
./src/backend/storage/buffer/localbuf.c-if (!LocalBufferDescriptors || 
!LocalBufferBlockPointers || !LocalRefCount)
./src/backend/storage/buffer/localbuf.c-ereport(FATAL,
./src/backend/storage/buffer/localbuf.c-
(errcode(ERRCODE_OUT_OF_MEMORY),
./src/backend/storage/buffer/localbuf.c- errmsg("out of 
memory")));
./src/backend/storage/buffer/localbuf.c-

./src/interfaces/libpq/fe-print.c-  nTups = PQntuples(res);
./src/interfaces/libpq/fe-print.c:  if (!(fieldNames = (const char **) 
calloc(nFields, sizeof(char *
./src/interfaces/libpq/fe-print.c-  {
./src/interfaces/libpq/fe-print.c-  fprintf(stderr, libpq_gettext("out 
of memory\n"));
./src/interfaces/libpq/fe-print.c-  abort();
./src/interfaces/libpq/fe-print.c-  }
./src/interfaces/libpq/fe-print.c:  if (!(fieldNotNum = (unsigned char *) 
calloc(nFields, 1)))
./src/interfaces/libpq/fe-print.c-  {
./src/interfaces/libpq/fe-print.c-  fprintf(stderr, libpq_gettext("out 
of memory\n"));
./src/interfaces/libpq/fe-print.c-  abort();
./src/interfaces/libpq/fe-print.c-  }
./src/interfaces/libpq/fe-print.c:  if (!(fieldMax = (int *) 
calloc(nFields, sizeof(int
./src/interfaces/libpq/fe-print.c-  {
./src/interfaces/libpq/fe-print.c-  fprintf(stderr, libpq_gettext("out 
of memory\n"));
./src/interfaces/libpq/fe-print.c-  abort();
./src/interfaces/libpq/fe-print.c-  }


Two identical pieces of code:

./src/backend/bootstrap/bootstrap.c-scan = 
heap_beginscan_catalog(rel, 0, NULL);
./src/backend/bootstrap/bootstrap.c-i = 0;
./src/backend/bootstrap/bootstrap.c-while ((tup = 
heap_getnext(scan, ForwardScanDirection)) != NULL)
./src/backend/bootstrap/bootstrap.c-++i;
./src/backend/bootstrap/bootstrap.c-heap_endscan(scan);
./src/backend/bootstrap/bootstrap.c:app = Typ = ALLOC(struct typmap 
*, i + 1);
./src/backend/bootstrap/bootstrap.c-while (i-- > 0)
./src/backend/bootstrap/bootstrap.c:*app++ = ALLOC(struct 
typmap, 1);
./src/backend/bootstrap/bootstrap.c-*app = NULL;
./src/backend/bootstrap/bootstrap.c-scan = 
heap_beginscan_catalog(rel, 0, NULL);
./src/backend/bootstrap/bootstrap.c-app = Typ;
./src/backend/bootstrap/bootstrap.c-while ((tup = 
heap_getnext(scan, ForwardScanDirection)) != NULL)
./src/backend/bootstrap/bootstrap.c-{


./src/backend/bootstrap/bootstrap.c-scan = 
heap_beginscan_catalog(rel, 0, NULL);
./src/backend/bootstrap/bootstrap.c-i = 0;
./src/backend/bootstrap/bootstrap.c-while ((tup = 
heap_getnext(scan, ForwardScanDirection)) != NULL)
./src/backend/bootstrap/bootstrap.c-++i;
./src/backend/bootstrap/bootstrap.c-heap_endscan(scan);
./src/backend/bootstrap/bootstrap.c:app = Typ = ALLOC(struct typmap 
*, i + 1);
./src/backend/bootstrap/bootstrap.c-while (i-- > 0)
./src/backend/bootstrap/bootstrap.c:*app++ = ALLOC(struct 
typmap, 1);
./src/backend/bootstrap/bootstrap.c-*app = NULL;
./src/backend/bootstrap/bootstrap.c-scan = 
heap_beginscan_catalog(rel, 0, NULL);
./src/backend/bootstrap/bootstrap.c-app = Typ;
./src/backend/bootstrap/bootstrap.c-while ((tup = 
heap_getnext(scan, ForwardScanDirection)) != NULL)
./src/backend/bootstrap/bootstrap.c-{

-- 
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] Missing checks when malloc returns NULL...

2016-08-29 Thread Michael Paquier 
On Sat, Aug 27, 2016 at 10:24 PM, Michael Paquier
 wrote:
> ./src/backend/postmaster/postmaster.c:  userDoption =
> strdup(optarg);
> [...]
> ./src/backend/bootstrap/bootstrap.c:userDoption =
> strdup(optarg);
> [...]
> ./src/backend/tcop/postgres.c:  userDoption = strdup(optarg);
> We cannot use pstrdup here because memory contexts are not set up
> here, still it would be better than crashing, but I am not sure if
> that's worth complicating this code.. Other opinions are welcome.

Those are not changed. We could have some NULL-checks but I am not
sure that's worth the complication here.

> ./contrib/vacuumlo/vacuumlo.c:  param.pg_user = strdup(optarg);
> [...]
> ./contrib/pg_standby/pg_standby.c:  triggerPath = strdup(optarg);
> [...]
> ./src/bin/pg_archivecleanup/pg_archivecleanup.c:
> additional_ext = strdup(optarg);/* Extension to remove
> Right we can do something here with pstrdup().

Those are updated with pg_strdup().

> ./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *)
> malloc(sizeof(SPIPlanPtr));
> Regarding refint.c, you can see upthread. Instead of reworking the
> code it would be better to just drop it from the tree.

I'd rather see this nuked out of the surface of the code tree.

> ./src/backend/utils/adt/pg_locale.c:grouping = strdup(extlconv->grouping);
> Here that would be a failure with an elog() as this is getting used by
> things like NUM_TOCHAR_finish and PGLC_localeconv.

Done.

> ./src/pl/tcl/pltcl.c:   prodesc->user_proname =
> strdup(NameStr(procStruct->proname));
> ./src/pl/tcl/pltcl.c:   prodesc->internal_proname =
> strdup(internal_proname);
> ./src/pl/tcl/pltcl.c-   if (prodesc->user_proname == NULL ||
> prodesc->internal_proname == NULL)
> ./src/pl/tcl/pltcl.c-   ereport(ERROR,
> ./src/pl/tcl/pltcl.c-   (errcode(ERRCODE_OUT_OF_MEMORY),
> ./src/pl/tcl/pltcl.c-errmsg("out of memory")));
> Ah, yes. Here we need to do a free(prodesc) first.

Done.

> ./src/common/exec.c:putenv(strdup(env_path));
> set_pglocale_pgservice() is used at the beginning of each process run,
> meaning that a failure here would be printf(stderr), followed by
> exit() for frontend, even ECPG as this compiles with -DFRONTEND.
> Backend can use elog(ERROR) btw.

Done.

Regarding the handling of strdup in libpq the code is careful in its
handling after a review, so we had better do nothing.

After that, there are two calls to realloc and one call to malloc that
deserve some attention, though those happen in pgc.l and I am not
exactly sure how to handle them.

As Alexander's email
(https://www.postgresql.org/message-id/20160826153358.GA29981%40e733)
has broken this thread, I am attaching to this thread the analysis
report that has been generated by Alexander previously. It was
referenced in only an URL.

Attached is an updated patch addressing those extra points.
-- 
Michael
find . -type f -iname '*.c' -exec egrep -C5 '[^a-z_](malloc|realloc|strdup)' {} 
+ | less

/(malloc|realloc|strdup)




./contrib/pg_standby/pg_standby.c-  case 't':   /* Trigger file 
*/
./contrib/pg_standby/pg_standby.c:  triggerPath = strdup(optarg);
./contrib/pg_standby/pg_standby.c-  break;

./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *) 
malloc(sizeof(SPIPlanPtr));
./contrib/spi/refint.c- *(plan->splan) = pplan;
./contrib/spi/refint.c- plan->nplans = 1;

./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *) malloc(nrefs * 
sizeof(SPIPlanPtr));
./contrib/spi/refint.c-
./contrib/spi/refint.c- for (r = 0; r < nrefs; r++)
./contrib/spi/refint.c- {
./contrib/spi/refint.c- relname = args2[0];
./contrib/spi/refint.c-

./contrib/spi/refint.c- if (strcmp((*eplan)[i].ident, ident) == 0)
./contrib/spi/refint.c- break;
./contrib/spi/refint.c- }
./contrib/spi/refint.c- if (i != *nplans)
./contrib/spi/refint.c- return (*eplan + i);
./contrib/spi/refint.c: *eplan = (EPlan *) realloc(*eplan, (i + 1) * 
sizeof(EPlan));
./contrib/spi/refint.c- newp = *eplan + i;
./contrib/spi/refint.c- }
./contrib/spi/refint.c- else
./contrib/spi/refint.c- {
./contrib/spi/refint.c: newp = *eplan = (EPlan *) malloc(sizeof(EPlan));
./contrib/spi/refint.c- (*nplans) = i = 0;
./contrib/spi/refint.c- }
./contrib/spi/refint.c-
./contrib/spi/refint.c- newp->ident = strdup(ident);
./contrib/spi/refint.c- newp->nplans = 0;

./contrib/spi/timetravel.c- if (i != *nplans)
./contrib/spi/timetravel.c- return (*eplan + i);
./contrib/spi/timetravel.c: *eplan = (EPlan *) realloc(*eplan, (i + 1) * 
sizeof(EPlan));
./contrib/spi/timetravel.c- newp = *eplan + i;
./contrib/spi/timetravel.c- }
./contrib/spi/timetravel.c- else
./contrib/spi/timetravel.c- {
./contrib/spi/timetravel.c: newp = *eplan = (EPlan *) malloc(sizeof(EPlan));
./contrib/spi/timetravel.c-

Re: [HACKERS] Missing checks when malloc returns NULL...

2016-08-27 Thread Michael Paquier 
On Sat, Aug 27, 2016 at 12:33 AM, Aleksander Alekseev
 wrote:
> Your patch [1] was marked as "Needs review" so I decided to take a look.

Thanks for the input!

> It looks good to me. However I found dozens of places in PostgreSQL code
> that seem to have similar problem you are trying to fix [2]. As far as I
> understand these are only places left that don't check malloc/realloc/
> strdup return values properly. I thought maybe you will be willing to
> fix they too so we could forget about this problem forever.

So my lookup was still incomplete.

> If not I will be happy to do it. However in this case we need someone
> familiar with affected parts of the code who will be willing to re-check
> a new patch since I'm not filling particularly confident about how
> exactly errors should be handled in all these cases.

I'll do it and compress that in my patch.

> By the way maybe someone knows other procedures besides malloc, realloc
> and strdup that require special attention?

I don't know how you did it, but this email has visibly broken the
original thread. Did you change the topic name?

./src/backend/postmaster/postmaster.c:  userDoption =
strdup(optarg);
[...]
./src/backend/bootstrap/bootstrap.c:userDoption =
strdup(optarg);
[...]
./src/backend/tcop/postgres.c:  userDoption = strdup(optarg);
We cannot use pstrdup here because memory contexts are not set up
here, still it would be better than crashing, but I am not sure if
that's worth complicating this code.. Other opinions are welcome.

./contrib/vacuumlo/vacuumlo.c:  param.pg_user = strdup(optarg);
[...]
./contrib/pg_standby/pg_standby.c:  triggerPath = strdup(optarg);
[...]
./src/bin/pg_archivecleanup/pg_archivecleanup.c:
additional_ext = strdup(optarg);/* Extension to remove
Right we can do something here with pstrdup().

./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *)
malloc(sizeof(SPIPlanPtr));
Regarding refint.c, you can see upthread. Instead of reworking the
code it would be better to just drop it from the tree.

./src/backend/utils/adt/pg_locale.c:grouping = strdup(extlconv->grouping);
Here that would be a failure with an elog() as this is getting used by
things like NUM_TOCHAR_finish and PGLC_localeconv.

./src/backend/utils/mmgr/mcxt.c:node = (MemoryContext) malloc(needed);
You cannot do much here...

./src/timezone/zic.c:   lcltime = strdup(optarg);
This is upstream code, not worth worrying.

./src/pl/tcl/pltcl.c:   prodesc->user_proname =
strdup(NameStr(procStruct->proname));
./src/pl/tcl/pltcl.c:   prodesc->internal_proname =
strdup(internal_proname);
./src/pl/tcl/pltcl.c-   if (prodesc->user_proname == NULL ||
prodesc->internal_proname == NULL)
./src/pl/tcl/pltcl.c-   ereport(ERROR,
./src/pl/tcl/pltcl.c-   (errcode(ERRCODE_OUT_OF_MEMORY),
./src/pl/tcl/pltcl.c-errmsg("out of memory")));
Ah, yes. Here we need to do a free(prodesc) first.

./src/common/exec.c:putenv(strdup(env_path));
set_pglocale_pgservice() is used at the beginning of each process run,
meaning that a failure here would be printf(stderr), followed by
exit() for frontend, even ECPG as this compiles with -DFRONTEND.
Backend can use elog(ERROR) btw.
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-08-26 Thread Aleksander Alekseev 
Hello, Michael.

Your patch [1] was marked as "Needs review" so I decided to take a look.

It looks good to me. However I found dozens of places in PostgreSQL code
that seem to have similar problem you are trying to fix [2]. As far as I
understand these are only places left that don't check malloc/realloc/
strdup return values properly. I thought maybe you will be willing to
fix they too so we could forget about this problem forever.

If not I will be happy to do it. However in this case we need someone
familiar with affected parts of the code who will be willing to re-check
a new patch since I'm not filling particularly confident about how
exactly errors should be handled in all these cases.

By the way maybe someone knows other procedures besides malloc, realloc
and strdup that require special attention?

[1] https://commitfest.postgresql.org/10/653/
[2] http://afiskon.ru/s/15/83287ef7d2_malloc.txt

-- 
Best regards,
Aleksander Alekseev


-- 
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] Missing checks when malloc returns NULL...

2016-08-18 Thread Michael Paquier 
On Thu, Aug 18, 2016 at 6:12 PM, Heikki Linnakangas  wrote:
> On 06/22/2016 04:41 AM, Michael Paquier wrote:
>> make s
>> OK, there is not much that we can do here then. What about the rest?
>> Those seem like legit concerns to me.
>
>
> There's also a realloc() and an strdup() call in refint.c. But looking at
> refint.c, I wonder why it's using malloc()/free() in the first place, rather
> than e.g. TopMemoryContext. The string construction code with sprintf() and
> strlen() looks quite antiqued, too, StringInfo would make it more readable.
>
> Does refint.c actually have any value anymore? What if we just removed it
> altogether? It's good to have some C triggers in contrib, to serve as
> examples, and to have some regression test coverage for all that. But ISTM
> that the 'autoinc' module covers the trigger API just as well.

I'd be in favor for nuking it instead of keeping this code as it
overlaps with autoinc, I did not notice that. Even if that's an
example, it is actually showing some bad code patters, which kills its
own purpose.

> The code in ps_status() runs before the elog machinery has been initialized,
> so you get a rather unhelpful error:
>
>> error occurred at ps_status.c:167 before error message processing is
>> available
>
> I guess that's still better than outright crashing, though. There are also a
> few strdup() calls there that can run out of memory..

Right. Another possibility is to directly call write_stderr to be sure
that the user gets the right message, then do exit(1). Thinking more
about it, as save_ps_display_args is called really at the beginning
this is perhaps what makes the most sense, so I changed the patch this
way.

> Not many of the callers of simple_prompt() check for a NULL result, so in
> all probability, returning NULL from there will just crash in the caller.
> There's one existing "return NULL" in there already, so this isn't a new
> problem, but can we find a better way?

I got inspired by the return value in the case of WIN32. Letting
sprompt.c in charge of printing an error does not seem like a good
idea to me, and there are already callers of simple_prompt() that
check for NULL and report an error appropriately, like pg_backup_db.c.
So I think that we had better adapt all the existing calls of
simple_prompt checking for NULL and make things more consistent by
having the callers report errors. Hence I updated the patch with those
changes.

Another possibility would be to keep a static buffer that has a fixed
size, basically 4096 as this is the maximum expected by psql, but
that's a waste of bytes in all other cases: one caller uses 4096, two
128 and the rest use 100 or less.

By the way, while looking at that, I also noticed that in exec_command
of psql's command.c we don't check for gets_fromFile that could return
NULL.

> There are more malloc(), realloc() and strdup() calls in isolationtester and
> pg_regress, that we ought to change too while we're at it.

Right. I added some handling for those callers as well with the pg_ equivalents.
-- 
Michael
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index e5eeec2..5dd0046 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -306,6 +306,11 @@ sql_conn(struct options * my_opts)
 		{
 			PQfinish(conn);
 			password = simple_prompt("Password: ", 100, false);
+			if (!password)
+			{
+fprintf(stderr, "%s: out of memory\n", "oid2name");
+exit(1);
+			}
 			new_pass = true;
 		}
 	} while (new_pass);
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 769c805..30b9ed2 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -71,7 +71,14 @@ vacuumlo(const char *database, const struct _param * param)
 
 	/* Note: password can be carried over from a previous call */
 	if (param->pg_prompt == TRI_YES && password == NULL)
+	{
 		password = simple_prompt("Password: ", 100, false);
+		if (!password)
+		{
+			fprintf(stderr, "out of memory\n");
+			return -1;
+		}
+	}
 
 	/*
 	 * Start the connection.  Loop until we have a password if requested by
@@ -115,6 +122,11 @@ vacuumlo(const char *database, const struct _param * param)
 		{
 			PQfinish(conn);
 			password = simple_prompt("Password: ", 100, false);
+			if (!password)
+			{
+fprintf(stderr, "out of memory\n");
+return -1;
+			}
 			new_pass = true;
 		}
 	} while (new_pass);
diff --git a/src/backend/port/dynloader/darwin.c b/src/backend/port/dynloader/darwin.c
index ccd92c3..a83c614 100644
--- a/src/backend/port/dynloader/darwin.c
+++ b/src/backend/port/dynloader/darwin.c
@@ -78,6 +78,9 @@ pg_dlsym(void *handle, char *funcname)
 	NSSymbol symbol;
 	char	   *symname = (char *) malloc(strlen(funcname) + 2);
 
+	if (!symname)
+		return NULL;
+
 	sprintf(symname, "_%s", funcname);
 	if (NSIsSymbolNameDefined(symname))
 	{
diff --git a/src/backend/utils/misc/ps_status.c b/src/backend/utils/misc/ps_status.c
index 892a810..09ef2df 100644
---

Re: [HACKERS] Missing checks when malloc returns NULL...

2016-08-18 Thread Heikki Linnakangas 

On 06/22/2016 04:41 AM, Michael Paquier wrote:

On Tue, Jun 21, 2016 at 10:46 PM, Tom Lane  wrote:

Michael Paquier  writes:

- mcxt.c uses that, which is surprising:
@@ -704,7 +704,8 @@ MemoryContextCreate(NodeTag tag, Size size,
{
/* Special case for startup: use good ol' malloc */
node = (MemoryContext) malloc(needed);
-   Assert(node != NULL);
+   if (node == NULL)
+   elog(PANIC, "out of memory");
}
I think that a PANIC is cleaner here instead of a simple crash.


But the elog mechanism assumes that memory contexts are working.
If this ever actually did fire, no good would come of it.

make s
OK, there is not much that we can do here then. What about the rest?
Those seem like legit concerns to me.


There's also a realloc() and an strdup() call in refint.c. But looking 
at refint.c, I wonder why it's using malloc()/free() in the first place, 
rather than e.g. TopMemoryContext. The string construction code with 
sprintf() and strlen() looks quite antiqued, too, StringInfo would make 
it more readable.


Does refint.c actually have any value anymore? What if we just removed 
it altogether? It's good to have some C triggers in contrib, to serve as 
examples, and to have some regression test coverage for all that. But 
ISTM that the 'autoinc' module covers the trigger API just as well.



The code in ps_status() runs before the elog machinery has been 
initialized, so you get a rather unhelpful error:



error occurred at ps_status.c:167 before error message processing is available


I guess that's still better than outright crashing, though. There are 
also a few strdup() calls there that can run out of memory..


Not many of the callers of simple_prompt() check for a NULL result, so 
in all probability, returning NULL from there will just crash in the 
caller. There's one existing "return NULL" in there already, so this 
isn't a new problem, but can we find a better way?


There are more malloc(), realloc() and strdup() calls in isolationtester 
and pg_regress, that we ought to change too while we're at it.


- Heikki



--
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] Missing checks when malloc returns NULL...

2016-06-23 Thread Michael Paquier 
On Wed, Jun 22, 2016 at 10:41 AM, Michael Paquier
 wrote:
> OK, there is not much that we can do here then. What about the rest?
> Those seem like legit concerns to me.

Registered this one to the next CF as a bug fix:
https://commitfest.postgresql.org/10/653/
It would be better not to crash if we can avoid it.
-- 
Michael


-- 
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] Missing checks when malloc returns NULL...

2016-06-21 Thread Michael Paquier 
On Tue, Jun 21, 2016 at 10:46 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> - mcxt.c uses that, which is surprising:
>> @@ -704,7 +704,8 @@ MemoryContextCreate(NodeTag tag, Size size,
>> {
>> /* Special case for startup: use good ol' malloc */
>> node = (MemoryContext) malloc(needed);
>> -   Assert(node != NULL);
>> +   if (node == NULL)
>> +   elog(PANIC, "out of memory");
>> }
>> I think that a PANIC is cleaner here instead of a simple crash.
>
> But the elog mechanism assumes that memory contexts are working.
> If this ever actually did fire, no good would come of it.

OK, there is not much that we can do here then. What about the rest?
Those seem like legit concerns to me.
-- 
Michael


malloc-nulls-v2.patch
Description: invalid/octet-stream

-- 
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] Missing checks when malloc returns NULL...

2016-06-21 Thread Tom Lane 
Michael Paquier  writes:
> - mcxt.c uses that, which is surprising:
> @@ -704,7 +704,8 @@ MemoryContextCreate(NodeTag tag, Size size,
> {
> /* Special case for startup: use good ol' malloc */
> node = (MemoryContext) malloc(needed);
> -   Assert(node != NULL);
> +   if (node == NULL)
> +   elog(PANIC, "out of memory");
> }
> I think that a PANIC is cleaner here instead of a simple crash.

But the elog mechanism assumes that memory contexts are working.
If this ever actually did fire, no good would come of 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