librelist archives

« back to archive

[PATCH] add assertions to avoid dereferencing NULL upon OOM

[PATCH] add assertions to avoid dereferencing NULL upon OOM

From:
Jim Meyering
Date:
2011-04-05 @ 10:26
Hello,

I noticed that out-of-memory (OOM) errors would cause
yajl code to dereference NULL (e.g., upon failed malloc and realloc).
Here's a patch that improves slightly on that by making yajl
fail with an assertion rather than dereferencing NULL.

Like the preceding patch, this is relative to the latest in git.

Jim


From f4a8048e803f9391c231a1dbf40052beb9338786 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Tue, 5 Apr 2011 10:58:58 +0200
Subject: [PATCH 2/2] add assertions to avoid dereferencing NULL upon OOM

---
 src/yajl.c        |    1 +
 src/yajl_buf.c    |    3 +++
 src/yajl_gen.c    |    2 ++
 src/yajl_lex.c    |    1 +
 src/yajl_parser.c |    2 ++
 5 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/src/yajl.c b/src/yajl.c
index 0345814..91f9f61 100644
--- a/src/yajl.c
+++ b/src/yajl.c
@@ -83,6 +83,7 @@ yajl_alloc(const yajl_callbacks * callbacks,
     }

     hand = (yajl_handle) YA_MALLOC(afs, sizeof(struct yajl_handle_t));
+    assert (hand != NULL);

     /* copy in pointers to allocation routines */
     memcpy((void *) &(hand->alloc), (void *) afs, sizeof(yajl_alloc_funcs));
diff --git a/src/yajl_buf.c b/src/yajl_buf.c
index c7e3796..62e305d 100644
--- a/src/yajl_buf.c
+++ b/src/yajl_buf.c
@@ -56,6 +56,7 @@ void yajl_buf_ensure_available(yajl_buf buf, unsigned int want)
     if (buf->data == NULL) {
         buf->len = YAJL_BUF_INIT_SIZE;
         buf->data = (unsigned char *) YA_MALLOC(buf->alloc, buf->len);
+        assert(buf->data != NULL);
         buf->data[0] = 0;
     }

@@ -73,6 +74,7 @@ void yajl_buf_ensure_available(yajl_buf buf, unsigned int want)

     if (need != buf->len) {
         buf->data = (unsigned char *) YA_REALLOC(buf->alloc, buf->data, need);
+        assert(buf->data != NULL);
         buf->len = need;
     }
 }
@@ -80,6 +82,7 @@ void yajl_buf_ensure_available(yajl_buf buf, unsigned int want)
 yajl_buf yajl_buf_alloc(yajl_alloc_funcs * alloc)
 {
     yajl_buf b = YA_MALLOC(alloc, sizeof(struct yajl_buf_t));
+    assert(b != NULL);
     memset((void *) b, 0, sizeof(struct yajl_buf_t));
     b->alloc = alloc;
     return b;
diff --git a/src/yajl_gen.c b/src/yajl_gen.c
index 548d948..85a8ec5 100644
--- a/src/yajl_gen.c
+++ b/src/yajl_gen.c
@@ -38,6 +38,7 @@
 #include <string.h>
 #include <stdio.h>
 #include <math.h>
+#include <assert.h>

 typedef enum {
     yajl_gen_start,
@@ -90,6 +91,7 @@ yajl_gen_alloc2(const yajl_print_t callback,
     }

     g = (yajl_gen) YA_MALLOC(afs, sizeof(struct yajl_gen_t));
+    assert(g != NULL);
     memset((void *) g, 0, sizeof(struct yajl_gen_t));
     /* copy in pointers to allocation routines */
     memcpy((void *) &(g->alloc), (void *) afs, sizeof(yajl_alloc_funcs));
diff --git a/src/yajl_lex.c b/src/yajl_lex.c
index 11e5f7b..0bae225 100644
--- a/src/yajl_lex.c
+++ b/src/yajl_lex.c
@@ -121,6 +121,7 @@ yajl_lex_alloc(yajl_alloc_funcs * alloc,
                unsigned int allowComments, unsigned int validateUTF8)
 {
     yajl_lexer lxr = (yajl_lexer) YA_MALLOC(alloc, sizeof(struct yajl_lexer_t));
+    assert(lxr != NULL);
     memset((void *) lxr, 0, sizeof(struct yajl_lexer_t));
     lxr->buf = yajl_buf_alloc(alloc);
     lxr->allowComments = allowComments;
diff --git a/src/yajl_parser.c b/src/yajl_parser.c
index 7fae6c6..861cd36 100644
--- a/src/yajl_parser.c
+++ b/src/yajl_parser.c
@@ -74,6 +74,7 @@ yajl_render_error_string(yajl_handle hand, const 
unsigned char * jsonText,
             memneeded += strlen(errorText);            
         }
         str = (unsigned char *) YA_MALLOC(&(hand->alloc), memneeded + 2);
+        assert(str != NULL);
         str[0] = 0;
         strcat((char *) str, errorType);
         strcat((char *) str, " error");    
@@ -114,6 +115,7 @@ yajl_render_error_string(yajl_handle hand, const 
unsigned char * jsonText,
                 YA_MALLOC(&(hand->alloc), (strlen((char *) str) +
                                            strlen((char *) text) +
                                            strlen(arrow) + 1));
+            assert(newStr != NULL);
             newStr[0] = 0;
             strcat((char *) newStr, (char *) str);
             strcat((char *) newStr, text);
-- 
1.7.5.rc0.351.gebf36

Re: [yajl] [PATCH] add assertions to avoid dereferencing NULL upon OOM

From:
Kai-Uwe Behrmann
Date:
2011-04-05 @ 10:34
Am 05.04.2011 12:26, schrieb Jim Meyering:
> I noticed that out-of-memory (OOM) errors would cause
> yajl code to dereference NULL (e.g., upon failed malloc and realloc).
> Here's a patch that improves slightly on that by making yajl
> fail with an assertion rather than dereferencing NULL.

The problem statement is quite valid. However the solution to add
asserts is in many environments not nice. Could you change your patch to 
return error values or give an error message and return where appropriate?

kind regards
Kai-Uwe Behrmann
-- 
open source colour management
www.behrmann.name

Re: [yajl] [PATCH] add assertions to avoid dereferencing NULL upon OOM

From:
Jim Meyering
Date:
2011-04-05 @ 11:11
Kai-Uwe Behrmann wrote:
> Am 05.04.2011 12:26, schrieb Jim Meyering:
>> I noticed that out-of-memory (OOM) errors would cause
>> yajl code to dereference NULL (e.g., upon failed malloc and realloc).
>> Here's a patch that improves slightly on that by making yajl
>> fail with an assertion rather than dereferencing NULL.
>
> The problem statement is quite valid. However the solution to add
> asserts is in many environments not nice. Could you change your patch to
> return error values or give an error message and return where appropriate?

You're asking a lot of a first-time contributor ;-)
Let's consider the assertion I added in yajl_gen_alloc2:

    yajl_gen
    yajl_gen_alloc(const yajl_gen_config * config,
                   const yajl_alloc_funcs * afs)
    {
        return yajl_gen_alloc2(NULL, config, afs, NULL);
    }

    yajl_gen
    yajl_gen_alloc2(const yajl_print_t callback,
                    const yajl_gen_config * config,
                    const yajl_alloc_funcs * afs,
                    void * ctx)
    {
    ...
        g = (yajl_gen) YA_MALLOC(afs, sizeof(struct yajl_gen_t));
        assert(g != NULL);
        memset((void *) g, 0, sizeof(struct yajl_gen_t));


If instead, we were to return NULL, we'd have to
exercise due diligence and ensure that all callers
handle a NULL return, even though there is already
the possibility of a NULL return from that function.

yajl_gen_alloc is the sole caller, and it merely returns the result,
so check its callers:

I see only one use, in reformatter/json_reformat.c:

    g = yajl_gen_alloc(&conf, NULL);

    /* ok.  open file.  let's read and parse */
    hand = yajl_alloc(&callbacks, &cfg, NULL, (void *) g);

and it fails to check for failed yajl_gen_alloc.
----------------------------------------

When returning early you have to be careful
also to free any previously-allocated temporaries.
In the case of yajl_gen_alloc2, there are none,
so indeed, a simple "return NULL;" would be better,
assuming the caller and the example in the manual
are also fixed.

That was just one example.
There are seven more.

I suggest you apply the patch I sent and treat each
assert statement like a FIXME comment, in that it is
something that should be removed ASAP.  Then, people
can fix each of them, a patch at a time.

In any case,
an assertion is always better than a NULL dereference.

Also consider the assertion I added in the infloop-fixing patch.
There, the containing function was of type "void".
It would have to be changed to return a value
and all callers updated.
Such changes should be made one at a time, and very carefully,
while the assert-instead-of-NULL-deref is a relatively clean,
targeted and clearly-safe change.

Here's an adjusted patch: in yajl_gen.c, I've
made it return NULL rather than adding an assert statement.
and of course, no longer include <assert.h>.
I presume someone else will take care of ensuring that
yajl_gen_alloc's return value is no longer ignored.

Jim

From 5f1bf196b800dd08dbdec2af1d189d5bc196e572 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Tue, 5 Apr 2011 10:58:58 +0200
Subject: [PATCH] don't dereference NULL upon OOM

---
 src/yajl.c        |    1 +
 src/yajl_buf.c    |    3 +++
 src/yajl_gen.c    |    2 ++
 src/yajl_lex.c    |    1 +
 src/yajl_parser.c |    2 ++
 5 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/src/yajl.c b/src/yajl.c
index 0345814..91f9f61 100644
--- a/src/yajl.c
+++ b/src/yajl.c
@@ -83,6 +83,7 @@ yajl_alloc(const yajl_callbacks * callbacks,
     }

     hand = (yajl_handle) YA_MALLOC(afs, sizeof(struct yajl_handle_t));
+    assert (hand != NULL);

     /* copy in pointers to allocation routines */
     memcpy((void *) &(hand->alloc), (void *) afs, sizeof(yajl_alloc_funcs));
diff --git a/src/yajl_buf.c b/src/yajl_buf.c
index c7e3796..62e305d 100644
--- a/src/yajl_buf.c
+++ b/src/yajl_buf.c
@@ -56,6 +56,7 @@ void yajl_buf_ensure_available(yajl_buf buf, unsigned int want)
     if (buf->data == NULL) {
         buf->len = YAJL_BUF_INIT_SIZE;
         buf->data = (unsigned char *) YA_MALLOC(buf->alloc, buf->len);
+        assert(buf->data != NULL);
         buf->data[0] = 0;
     }

@@ -73,6 +74,7 @@ void yajl_buf_ensure_available(yajl_buf buf, unsigned int want)

     if (need != buf->len) {
         buf->data = (unsigned char *) YA_REALLOC(buf->alloc, buf->data, need);
+        assert(buf->data != NULL);
         buf->len = need;
     }
 }
@@ -80,6 +82,7 @@ void yajl_buf_ensure_available(yajl_buf buf, unsigned int want)
 yajl_buf yajl_buf_alloc(yajl_alloc_funcs * alloc)
 {
     yajl_buf b = YA_MALLOC(alloc, sizeof(struct yajl_buf_t));
+    assert(b != NULL);
     memset((void *) b, 0, sizeof(struct yajl_buf_t));
     b->alloc = alloc;
     return b;
diff --git a/src/yajl_gen.c b/src/yajl_gen.c
index 548d948..e08712b 100644
--- a/src/yajl_gen.c
+++ b/src/yajl_gen.c
@@ -90,6 +90,8 @@ yajl_gen_alloc2(const yajl_print_t callback,
     }

     g = (yajl_gen) YA_MALLOC(afs, sizeof(struct yajl_gen_t));
+    if (g == NULL)
+        return NULL;
     memset((void *) g, 0, sizeof(struct yajl_gen_t));
     /* copy in pointers to allocation routines */
     memcpy((void *) &(g->alloc), (void *) afs, sizeof(yajl_alloc_funcs));
diff --git a/src/yajl_lex.c b/src/yajl_lex.c
index 11e5f7b..0bae225 100644
--- a/src/yajl_lex.c
+++ b/src/yajl_lex.c
@@ -121,6 +121,7 @@ yajl_lex_alloc(yajl_alloc_funcs * alloc,
                unsigned int allowComments, unsigned int validateUTF8)
 {
     yajl_lexer lxr = (yajl_lexer) YA_MALLOC(alloc, sizeof(struct yajl_lexer_t));
+    assert(lxr != NULL);
     memset((void *) lxr, 0, sizeof(struct yajl_lexer_t));
     lxr->buf = yajl_buf_alloc(alloc);
     lxr->allowComments = allowComments;
diff --git a/src/yajl_parser.c b/src/yajl_parser.c
index 7fae6c6..861cd36 100644
--- a/src/yajl_parser.c
+++ b/src/yajl_parser.c
@@ -74,6 +74,7 @@ yajl_render_error_string(yajl_handle hand, const 
unsigned char * jsonText,
             memneeded += strlen(errorText);            
         }
         str = (unsigned char *) YA_MALLOC(&(hand->alloc), memneeded + 2);
+        assert(str != NULL);
         str[0] = 0;
         strcat((char *) str, errorType);
         strcat((char *) str, " error");    
@@ -114,6 +115,7 @@ yajl_render_error_string(yajl_handle hand, const 
unsigned char * jsonText,
                 YA_MALLOC(&(hand->alloc), (strlen((char *) str) +
                                            strlen((char *) text) +
                                            strlen(arrow) + 1));
+            assert(newStr != NULL);
             newStr[0] = 0;
             strcat((char *) newStr, (char *) str);
             strcat((char *) newStr, text);
-- 
1.7.5.rc0.351.gebf36

Re: [yajl] [PATCH] add assertions to avoid dereferencing NULL upon OOM

From:
Vikas N Kumar
Date:
2011-04-05 @ 13:24
On Tue, Apr 5, 2011 at 7:11 AM, Jim Meyering <jim@meyering.net> wrote:

> Let's consider the assertion I added in yajl_gen_alloc2:
>        assert(g != NULL);
>

I am only trying to point out the obvious here: assert() is a no-op in a
non-debug build on Linux. So assert() is wrong. If you really want to stop
execution, throw an exception.
if you are not willing to throw an exception, then return -1 like most other
POSIX libraries.

If the programmer is using a library and not checking return values for
errors, that is a problem in the programmer's code not in the library that
is informing the programmer that something went wrong.

Re: [yajl] [PATCH] add assertions to avoid dereferencing NULL upon OOM

From:
Jim Meyering
Date:
2011-04-05 @ 11:22
Kai-Uwe Behrmann wrote:
> Am 05.04.2011 12:26, schrieb Jim Meyering:
>> I noticed that out-of-memory (OOM) errors would cause
>> yajl code to dereference NULL (e.g., upon failed malloc and realloc).
>> Here's a patch that improves slightly on that by making yajl
>> fail with an assertion rather than dereferencing NULL.
>
> The problem statement is quite valid. However the solution to add
> asserts is in many environments not nice. Could you change your patch to
> return error values or give an error message and return where appropriate?

FYI, printing a diagnostic is inappropriate.
When a library like this fails in any way, it must
communicate its result via its API, not via stdout, stderr, etc.
Some client applications do not even have usable output streams.

Similarly, barring rare exceptions, a library must never exit or abort
(or get a failed assertion), because that would be most unwelcome in a
daemon-style application that is expected to run indefinitely.

Hence my suggestion to take the assert-adding patch, but
to remove them as soon as someone finds the time to rearrange
the code properly.