Bug 61332 - libobjc unsafe malloc use instead objc_malloc
Summary: libobjc unsafe malloc use instead objc_malloc
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: libobjc (show other bugs)
Version: unknown
: P3 major
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-27 19:33 UTC by Maksymilian Arciemowicz
Modified: 2014-06-16 12:11 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Maksymilian Arciemowicz 2014-05-27 19:33:30 UTC
Hi,

I have found interesting examples of dynamic memory allocation use in libobjc

libobjc/selector.c -----------------------------------------
SEL *
sel_copyTypedSelectorList (const char *name, unsigned int *numberOfReturnedSelectors)
{
  unsigned int count = 0;
  SEL *returnValue = NULL;
  sidx i;
   
  if (name == NULL)
    {
      if (numberOfReturnedSelectors)
    *numberOfReturnedSelectors = 0;
      return NULL;
    }
 
  objc_mutex_lock (__objc_runtime_mutex);
 
  /* Count how many selectors we have.  */
  i = (sidx) objc_hash_value_for_key (__objc_selector_hash, name);
  if (i != 0)
    {
      struct objc_list *selector_list = NULL;
      selector_list = (struct objc_list *) sarray_get_safe (__objc_selector_array, i);
 
      /* Count how many selectors we have.  */
      {
    struct objc_list *l;
    for (l = selector_list; l; l = l->tail)
      count++;
      }
 
      if (count != 0)
    {
      /* Allocate enough memory to hold them.  */
      returnValue = (SEL *)(malloc (sizeof (SEL) * (count + 1))); <===============
       
      /* Copy the selectors.  */
      {
        unsigned int j;
        for (j = 0; j < count; j++)
          {
        returnValue[j] = (SEL)(selector_list->head); <===========================
        selector_list = selector_list->tail;
          }
        returnValue[j] = NULL;
 libobjc/selector.c -----------------------------------------

 or 
 
 libobjc/ivars.c  -----------------------------------------
 struct objc_ivar ** class_copyIvarList (Class class_, unsigned int *numberOfReturnedIvars)
{
  unsigned int count = 0;
  struct objc_ivar **returnValue = NULL;
  struct objc_ivar_list* ivar_list;
 
  if (class_ == Nil  ||  CLS_IS_IN_CONSTRUCTION (class_))
    {
      if (numberOfReturnedIvars)
    *numberOfReturnedIvars = 0;
      return NULL;
    }
     
  /* Count how many ivars we have.  */
  ivar_list = class_->ivars;
  count = ivar_list->ivar_count;
 
  if (count != 0)
    {
      unsigned int i = 0;
       
      /* Allocate enough memory to hold them.  */
      returnValue = (struct objc_ivar **)(malloc (sizeof (struct objc_ivar *) * (count + 1))); <======
       
      /* Copy the ivars.  */
      for (i = 0; i < count; i++)
    returnValue[i] = &(ivar_list->ivar_list[i]); <===========================
       
      returnValue[i] = NULL;
    }
libobjc/ivars.c  -----------------------------------------  

 or

libobjc/protocols.c  -----------------------------------------
Protocol **
objc_copyProtocolList (unsigned int *numberOfReturnedProtocols)
{
  unsigned int count = 0;
  Protocol **returnValue = NULL;
  node_ptr node;
 
  objc_mutex_lock (__protocols_hashtable_lock);
 
  /* Count how many protocols we have.  */
  node = objc_hash_next (__protocols_hashtable, NULL);
  while (node)
    {
      count++;
      node = objc_hash_next (__protocols_hashtable, node);
    }
 
  if (count != 0)
    {
      unsigned int i = 0;
 
      /* Allocate enough memory to hold them.  */
      returnValue = (Protocol **)(malloc (sizeof (Protocol *) * (count + 1))); <===========================
       
      /* Copy the protocols.  */
      node = objc_hash_next (__protocols_hashtable, NULL); <===========================
      while (node)
    {
      returnValue[i] = node->value;
libobjc/protocols.c  -----------------------------------------


Why malloca() was used instead objc_malloc()? Is there any sensible reason?
In terms of safety, if you can create a properly buffer length (FREE_MEMORY / 2 +1) may end up to crash at this point. 

http://cwe.mitre.org/data/definitions/252.html

Thanks for taking the time.

Maksymilian
http://cifrex.org
Comment 1 Andrew Pinski 2014-05-27 20:18:36 UTC
The difference between malloc and objc_malloc is minor when not using GC.  In fact the program will crash is malloc returns NULL anyways.
Comment 2 Maksymilian Arciemowicz 2014-05-27 23:14:57 UTC
It seems that we have two problems here:
1 The first is memory allocation without GC_malloc (when GC used) 
2 If OBJC_WITH_GC is not definied, objc_malloc() also check result of malloc()

-----
 objc_malloc (size_t size)
{
  void *res = (void *)(malloc (size));
  if (! res)
    _objc_abort ("Virtual memory exhausted\n");
  return res;
}
-----

related to

https://www.securecoding.cert.org/confluence/display/cplusplus/EXP34-CPP.+Ensure+a+null+pointer+is+not+dereferenced

Thank you for your attention

Maksymilian Arciemowicz
http://cifrex.org