Re: Lack of Sanity Check for call to malloc()

[Todd Wease <twease () sourcefire com>]

Hello Bill,

Thanks for the code review.  Comments inline...

On Sat, Jul 20, 2013 at 4:13 PM, Bill Parker <wp02855 () gmail com> wrote:

> Hello All,
>
> In 'snort-2.9.5/src/preprocessors/HttpInspect/utils', file
> 'hi_paf.c', I found a call to malloc() without a check for a
> return value of NULL, indicating failure.  The patch file
> below adds this test:
>
> --- hi_paf.c.orig       2013-07-19 17:20:14.860817849 -0700
> +++ hi_paf.c    2013-07-19 17:24:00.310814409 -0700
> @@ -552,6 +552,12 @@
>      }
>      hi_fsm_size = max + extra;
>      hi_fsm = malloc(hi_fsm_size*sizeof(*hi_fsm));
> +    if (hi_fsm == NULL)
> +    {
> +       printf("Unable to allocate memory for hi_fsm...\n");
> +       return false;
> +    }
> +
>      next = max;
>
>      for ( i = 0; i < hi_fsm_size; i++ )

A bug will be created for this.


> In 'snort-2.9.5/src', file 'log.c', I found a check for
> the return value from calloc() but the call to calloc() in
> function 'PrintCharData' is currently commented out as
> shown below:
>
>     /* allocate a buffer to print the data to */
>     //data_dump_buffer = (char *) calloc(data_len + (data_len >> 6) + 2,
> sizeof(char));
>     if (data_dump_buffer == NULL)
>     {
>         AllocDumpBuf();
>     }
>
>     size = (data_len + (data_len >> 6) + 2) * sizeof(char);
> Should the variable 'data_dump_buffer' be uncommented, since
> data_dump_buffer is accessed later in the function?

data_dump_buffer is global and AllocDumpBuf() does the allocation.


> In 'snort-2.9.5/src/detection-plugins', file 'sp_ip_proto.c',
> I found a check for the return value from calloc(), but the call
> to calloc() in function 'IpProtoInit' is currently commented out
> as shown below:
>
>     ipd = (IpProtoData *) SnortAlloc(sizeof(IpProtoData));
>
>     /* allocate the data structure and attach it to the
>        rule's data struct list */
>     //otn->ds_list[PLUGIN_IP_PROTO_CHECK] = (IpProtoData *)
> calloc(sizeof(IpProtoData), sizeof(char));
>
>     /* this is where the keyword arguments are processed and placed into
> the
>        rule option's data structure */
>     IpProtoRuleParseFunction(data, ipd);
> Should the variable 'otn->ds_list[PLUGIN_IP_PROTO_CHECK]'
> be uncommented, since data_dump_buffer is accessed later
> in the function?

No. otn->ds_list[PLUGIN_IP_PROTO_CHECK] is set to ipd below.


> I'm attaching the patch file to this email.
>
> Bill (wp02855 at gmail dot com)
>
>
> ------------------------------------------------------------------------------
> See everything from the browser to the database with AppDynamics
> Get end-to-end visibility with application monitoring from AppDynamics
> Isolate bottlenecks and diagnose root cause in seconds.
> Start your free trial of AppDynamics Pro today!
> http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
> _______________________________________________
> Snort-devel mailing list
> Snort-devel () lists sourceforge net
> https://lists.sourceforge.net/lists/listinfo/snort-devel
> Archive:
> http://sourceforge.net/mailarchive/forum.php?forum_name=snort-devel
>
> Please visit http://blog.snort.org for the latest news about Snort!
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk

_______________________________________________
Snort-devel mailing list
Snort-devel () lists sourceforge net
https://lists.sourceforge.net/lists/listinfo/snort-devel
Archive:
http://sourceforge.net/mailarchive/forum.php?forum_name=snort-devel

Please visit http://blog.snort.org for the latest news about Snort!