queue_new(): if calloc fails, abort (CID #57918)
diff mbox

Message ID 1460644722-22972-1-git-send-email-nhofmeyr@sysmocom.de
State Not Applicable
Headers show

Commit Message

Neels Hofmeyr April 14, 2016, 2:38 p.m. UTC
Coverity complains about a 'Dereference before null check' on *queue.
So, push the NULL check further up, but also, instead of handling a calloc
failure as error, rather abort the program.
---
 gtp/queue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Harald Welte April 16, 2016, 11:37 a.m. UTC | #1
Hi Neels

On Thu, Apr 14, 2016 at 04:38:42PM +0200, Neels Hofmeyr wrote:
> Coverity complains about a 'Dereference before null check' on *queue.
> So, push the NULL check further up, 

No question here.

> but also, instead of handling a calloc failure as error, rather abort
> the program.

I think that's a much more fundamental question.  Should we really abort
the program in this case?  If so, why only in case of queue allocation
failures, but not in general at all memory allocation failures?  And if
that's the case, wrapping calloc() / malloc() and other dynamic memory
allocation calls with a function that contains the abort() (or an
OSMO_ASSERT() on the result) might be more applicable?
Neels Hofmeyr April 18, 2016, 10:56 a.m. UTC | #2
On Sat, Apr 16, 2016 at 01:37:41PM +0200, Harald Welte wrote:
> Hi Neels
> 
> On Thu, Apr 14, 2016 at 04:38:42PM +0200, Neels Hofmeyr wrote:
> > Coverity complains about a 'Dereference before null check' on *queue.
> > So, push the NULL check further up, 
> 
> No question here.
> 
> > but also, instead of handling a calloc failure as error, rather abort
> > the program.
> 
> I think that's a much more fundamental question.  Should we really abort
> the program in this case?

In an in-person discussion with Holger on some other code way back some day, he
recommended to abort() on allocation failure. Might not be applicable here, of
course.

> If so, why only in case of queue allocation
> failures, but not in general at all memory allocation failures?  And if
> that's the case, wrapping calloc() / malloc() and other dynamic memory
> allocation calls with a function that contains the abort() (or an
> OSMO_ASSERT() on the result) might be more applicable?

Yes, I would agree with that.

(BTW, the only reason I didn't OSMO_ASSERT() was that there is no other use of
OSMO_ASSERT() anywhere else in OpenGGSN.)

How should we handle this, I'd prefer not to spend time on that now. Commit the
patch with `return EOF' instead of abort()ing, as the old code suggests? I
don't know about that, EOF doesn't seem applicable at all.

~Neels
Holger Freyther April 22, 2016, 1:39 p.m. UTC | #3
> On 18 Apr 2016, at 12:56, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> 
> 
> (BTW, the only reason I didn't OSMO_ASSERT() was that there is no other use of
> OSMO_ASSERT() anywhere else in OpenGGSN.)

the GGSN has not used libosmocore before, so there will be many firsts.

> 
> How should we handle this, I'd prefer not to spend time on that now. Commit the
> patch with `return EOF' instead of abort()ing, as the old code suggests? I
> don't know about that, EOF doesn't seem applicable at all.

return EOF then. For commit messages we used to put "Fixes: CID#1234" at the bottom of the commit message.

kind regards
	holger
Neels Hofmeyr April 25, 2016, 11:01 a.m. UTC | #4
On Fri, Apr 22, 2016 at 03:39:20PM +0200, Holger Freyther wrote:
> return EOF then. For commit messages we used to put "Fixes: CID#1234" at the bottom of the commit message.

submitting a new patch with subject "queue_new(): fix NULL dereference on allocation failure"

~Neels

Patch
diff mbox

diff --git a/gtp/queue.c b/gtp/queue.c
index 7c971b0..707b522 100644
--- a/gtp/queue.c
+++ b/gtp/queue.c
@@ -127,16 +127,15 @@  int queue_new(struct queue_t **queue)
 	if (QUEUE_DEBUG)
 		printf("queue_new\n");
 	*queue = calloc(1, sizeof(struct queue_t));
+	if (!(*queue))
+		abort();
 	(*queue)->next = 0;
 	(*queue)->first = -1;
 	(*queue)->last = -1;
 
 	if (QUEUE_DEBUG)
 		queue_print(*queue);
-	if (*queue)
-		return 0;
-	else
-		return EOF;
+	return 0;
 }
 
 /*! \brief Deallocates queue structure */