linuxptp-users

[Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Bill P. <wp...@gm...>]

Hello All,

    In reviewing source code in linuxptp-1.5, I found several instances
where the return values for calls to malloc()/calloc()/strdup() are not
checked for a return value of NULL, indicating failure.  This is in file
'timemaster.c', and the patch file below should address these issues,
and also free()'s previously allocated memory in the order it was
allocated:

=======================================================================

--- timemaster.c.orig    2015-08-19 18:35:03.044000000 -0700
+++ timemaster.c    2015-08-19 18:57:01.046000000 -0700
@@ -268,9 +268,18 @@
     }

     source = malloc(sizeof(*source));
+    if (!source) {
+        pr_err("low memory, failed to add source to ntp_server");
+        return NULL;
+    }
     source->type = NTP_SERVER;
     source->ntp = ntp_server;
     source->ntp.address = strdup(source->ntp.address);
+    if (!source->ntp.address) {
+        pr_err("low memory, failed to add source->ntp.address");
+        free(source);
+        return NULL;
+    }

     return source;
 }
@@ -282,6 +291,10 @@
     int r = 0;

     source = malloc(sizeof(*source));
+    if (!source) {
+        pr_err("low memory, failed to add source to ptp_parse");
+        goto failed;
+    }
     source->type = PTP_DOMAIN;
     source->ptp.delay = DEFAULT_PTP_DELAY;
     source->ptp.ntp_poll = DEFAULT_PTP_NTP_POLL;
@@ -482,10 +495,19 @@
     char buf[4096], *line, *section_name = NULL;
     char **section_lines = NULL;
     int ret = 0;
-
+
+    if (!config) {
+        pr_err("low memory, failed to add config to config_parse");
+        return NULL;
+    }
     config->sources = (struct source **)parray_new();
     config->ntp_program = DEFAULT_NTP_PROGRAM;
     config->rundir = strdup(DEFAULT_RUNDIR);
+    if (!config->rundir) {
+        pr_err("low memory, failed to add config->rundir to config_parse");
+        free(config);
+        return NULL;
+    }

     init_program_config(&config->chronyd, "chronyd",
                 NULL, DEFAULT_CHRONYD_SETTINGS, NULL);
@@ -666,6 +688,10 @@

     /* get PHCs used by specified interfaces */
     phcs = malloc(num_interfaces * sizeof(int));
+    if (!phcs) {
+        pr_err("low memory, failed to add phcs to ptp_source");
+        return 0;
+    }
     for (i = 0; i < num_interfaces; i++) {
         phcs[i] = -1;

@@ -719,6 +745,11 @@

             /* don't use this PHC in other sources */
             phc = malloc(sizeof(int));
+            if (!phc) {
+                pr_err("low memory, failed to add phc to ptp_source");
+                free(phcs);
+                return 0;
+            }
             *phc = phcs[i];
             parray_append((void ***)allocated_phcs, phc);
         }
@@ -727,9 +758,22 @@
                        config->rundir, *shm_segment);

         config_file = malloc(sizeof(*config_file));
+        if (!config_file) {
+            pr_err("low memory, failed to add config_file to ptp_source");
+            free(phc);
+            free(phcs);
+            return 0;
+        }
         config_file->path = string_newf("%s/ptp4l.%d.conf",
                         config->rundir, *shm_segment);
         config_file->content = strdup("[global]\n");
+        if (!config_file->content) {
+            pr_err("low memory, failed to add config_file->content to
ptp_source");
+            free(config_file);
+            free(phc);
+            free(phcs);
+            return 0;
+        }
         extend_config_string(&config_file->content,
                      config->ptp4l.settings);
         extend_config_string(&config_file->content,
@@ -811,7 +855,17 @@
     struct config_file *ntp_config = malloc(sizeof(*ntp_config));
     char **command = NULL;

+    if (!ntp_config) {
+        pr_err("low memory, failed to add ntp_config to add_ntp_program");
+        return NULL;
+    }
+
     ntp_config->content = strdup("");
+    if (!ntp_config->content) {
+        pr_err("low memory, failed to add ntp_config->content to
add_ntp_program");
+        free(ntp_config);
+        return NULL;
+    }

     switch (config->ntp_program) {
     case CHRONYD:
@@ -866,6 +920,10 @@
     int **allocated_phcs = (int **)parray_new();
     int ret = 0, shm_segment = 0;

+    if (!script) {
+        pr_err("low memory, failed to add script to script_create");
+        return NULL;
+    }
     script->configs = (struct config_file **)parray_new();
     script->commands = (char ***)parray_new();

=======================================================================

running 'make' in the linuxptp-1.5 source tree (with the code added
above) results in a clean build, btw.

I am attaching the patch file (diff -u format) to this bug report...

Comments, Questions, Complaints, Suggestions? :)

Bill Parker (wp02855 at gmail dot com)

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Keller, J. E <jac...@in...>]

Hi Bill,

On Thu, 2015-08-20 at 14:51 -0700, Bill Parker wrote:
> Hello All,
> 
>     In reviewing source code in linuxptp-1.5, I found several
> instances
> where the return values for calls to malloc()/calloc()/strdup() are
> not
> checked for a return value of NULL, indicating failure.  This is in
> file
> 'timemaster.c', and the patch file below should address these issues,
> and also free()'s previously allocated memory in the order it was
> allocated:
> 

It is common practice for this list to use git-send-email to generate
the patch, rather than creating the email by hand. Then the subject of
the email could be the commit message. You can also use --- scissors
feature to include comments that may not be valid inside a commit
message but should appear on the mailing list. That being said, this
patch looks ok.

One general comment is for blocks which have early returns it may be
worth using a "goto err_xyz;" style instead of copying error
recovery/handling. This often can simplify the "undo the stuff we
already did as we exit" to reduce repeated code (which is a likely
place to add memory leaks in the future as code changes).

I couldn't easily tell if any of the blocks would really benefit from
this or not, but it is something to think about.

<snip>

> 
> running 'make' in the linuxptp-1.5 source tree (with the code added
> above) results in a clean build, btw.
> 
> I am attaching the patch file (diff -u format) to this bug report...
> 
> Comments, Questions, Complaints, Suggestions? :)


Thanks for the patch!

Regards,
Jake

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Miroslav L. <mli...@re...>]

Hi,

(this probably belongs to the linuxptp-devel list)

On Thu, Aug 20, 2015 at 02:51:04PM -0700, Bill Parker wrote:
> Hello All,
> 
>     In reviewing source code in linuxptp-1.5, I found several instances
> where the return values for calls to malloc()/calloc()/strdup() are not
> checked for a return value of NULL, indicating failure.  This is in file
> 'timemaster.c', and the patch file below should address these issues,
> and also free()'s previously allocated memory in the order it was
> allocated:

Is this to fix warnings from a static code analyzer?

There are other places in the timemaster code where memory is
allocated, e.g. the parray_* and string_* function calls. If the code
tried to handle them all, it would be a mess.

I think an easier and cleaner approach would be to write wrappers for
the strdup/calloc/malloc/realloc functions which just send an error
message and call exit(1) when the allocation fails, and use the
wrappers everywhere in the code.

-- 
Miroslav Lichvar

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Richard C. <ric...@gm...>]

On Fri, Aug 21, 2015 at 08:55:01AM +0200, Miroslav Lichvar wrote:
> I think an easier and cleaner approach would be to write wrappers for
> the strdup/calloc/malloc/realloc functions which just send an error
> message and call exit(1) when the allocation fails, and use the
> wrappers everywhere in the code.

If you want to do that way for timemaster, fine.

But for ptp4l/pmc/phc2sys, I prefer proper error handling.

Thanks,
Richard

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Keller, J. E <jac...@in...>]

On Fri, 2015-08-21 at 08:55 +0200, Miroslav Lichvar wrote:

> I think an easier and cleaner approach would be to write wrappers for
> the strdup/calloc/malloc/realloc functions which just send an error
> message and call exit(1) when the allocation fails, and use the
> wrappers everywhere in the code.
> 

Yea, I think this is probably a good call. We could use these
everywhere in the code base.

Regards,
Jake

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Bill P. <wp...@gm...>]

This was a manual review of the code, I did not use something like
clang-analyzer to find these :)

The wrapper approach makes sense as well (IMO).

Bill

On Fri, Aug 21, 2015 at 8:52 AM, Keller, Jacob E <jac...@in...>
wrote:

> On Fri, 2015-08-21 at 08:55 +0200, Miroslav Lichvar wrote:
>
> > I think an easier and cleaner approach would be to write wrappers for
> > the strdup/calloc/malloc/realloc functions which just send an error
> > message and call exit(1) when the allocation fails, and use the
> > wrappers everywhere in the code.
> >
>
> Yea, I think this is probably a good call. We could use these
> everywhere in the code base.
>
> Regards,
> Jake

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Xavier B. <xav...@fr...>]

Under linux malloc() will only return NULL when you get out of address 
space, i.e. virtually never on a 64bits machine. Low memory situations 
are handled elsewhere (e.g. OOM killer).
So I don't think checking malloc() for NULL is that wise.

     Xav

Le 21/08/2015 18:43, Bill Parker a écrit :
> This was a manual review of the code, I did not use something like 
> clang-analyzer to find these :)
>
> The wrapper approach makes sense as well (IMO).
>
> Bill
>
> On Fri, Aug 21, 2015 at 8:52 AM, Keller, Jacob E 
> <jac...@in... <mailto:jac...@in...>> wrote:
>
>     On Fri, 2015-08-21 at 08:55 +0200, Miroslav Lichvar wrote:
>
>     > I think an easier and cleaner approach would be to write
>     wrappers for
>     > the strdup/calloc/malloc/realloc functions which just send an error
>     > message and call exit(1) when the allocation fails, and use the
>     > wrappers everywhere in the code.
>     >
>
>     Yea, I think this is probably a good call. We could use these
>     everywhere in the code base.
>
>     Regards,
>     Jake
>
>
>
>
> ------------------------------------------------------------------------------
>
>
> _______________________________________________
> Linuxptp-users mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linuxptp-users

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Miroslav L. <mli...@re...>]

On Thu, Aug 27, 2015 at 04:06:23PM +0200, Xavier Bestel wrote:
> Under linux malloc() will only return NULL when you get out of address
> space, i.e. virtually never on a 64bits machine. Low memory situations are
> handled elsewhere (e.g. OOM killer).
> So I don't think checking malloc() for NULL is that wise.

It depends on the kernel overcommit configuration. In the default
configuration getting NULL is unlikely, but that doesn't mean it
should result in a segfault. I'm just not convinced there is any point
in trying to recover from that.

-- 
Miroslav Lichvar

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Keller, J. E <jac...@in...>]

> -----Original Message-----
> From: Miroslav Lichvar [mailto:mli...@re...]
> Sent: Thursday, August 27, 2015 8:16 AM
> To: lin...@li...
> Subject: Re: [Linuxptp-users] Missing Sanity Checks for
> malloc()/calloc()/strdup() in linuxptp-1.5
> 
> On Thu, Aug 27, 2015 at 04:06:23PM +0200, Xavier Bestel wrote:
> > Under linux malloc() will only return NULL when you get out of address
> > space, i.e. virtually never on a 64bits machine. Low memory situations are
> > handled elsewhere (e.g. OOM killer).
> > So I don't think checking malloc() for NULL is that wise.
> 
> It depends on the kernel overcommit configuration. In the default
> configuration getting NULL is unlikely, but that doesn't mean it
> should result in a segfault. I'm just not convinced there is any point
> in trying to recover from that.
> 

Generally, no, and I would prefer we at least try and die gracefully, and if *every* place we check will die gracefully, then why force each end point to die itself?

By die gracefully here, I mean not segfault, but just instantly quit.

Regards,
Jake

Re: [Linuxptp-users] Missing Sanity Checks for malloc()/calloc()/strdup() in linuxptp-1.5

[Richard C. <ric...@gm...>]

On Tue, Sep 01, 2015 at 09:34:32PM +0000, Keller, Jacob E wrote:
> By die gracefully here, I mean not segfault, but just instantly quit.

Right, and Miroslav's recent patches do catch a malloc failure and
call exit() in that case.

Thanks,
Richard