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. >

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

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

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

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

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

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

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

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 =

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

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

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

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,

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

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

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

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

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

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 >

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

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

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

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 >

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

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); > [...] >

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

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

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()

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, { /*

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

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'

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); > -

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

2016-06-20 Thread Michael Paquier
Hi all, While auditing the code, I got surprised that there are a couple of code paths that do nothing for this error handling: - pg_regress and isolationtester use malloc extensively, in case of failure those would just crash crash. I think that it matters for buildfarm members that are under