Re: [C] please critique my code

From: B. v Ingen Schenau (bart_at_ingen.ddns.info)
Date: 06/09/04 

Date: Wed, 09 Jun 2004 18:26:49 +0200

Elliot Marks wrote:

> The function converts decimal to binary format. In the do loop,
> calls to realloc grow the binary string 1 byte at a time as long
> as there is another bit to add. This technique seems to work but
> I am self-taught and would appreciate expert advice whether there
> are potential problems lurking in this code that I have not taken
> into account. (bin is freed in the caller.)
>
> char* dec2bin(int n)

Because you are using signed int, and due to the way you implemented the
algorithm, you are limiting the range of values that can be processed by
this function.
With the current implementation, you can only handle values in the range
[0 .. INT_MAX]. To handle values in the range [INT_MIN .. UINT_MAX], you
should make n an unsigned int, and mask an unsigned long. (The negative
values will be represented in twos-complement, which most likely is also
the internal representation used by your system)
If you wish to handle the range [LONG_MIN .. ULONG_MAX], then you need
special code to handle the case where the high bit is set.

> {
> int mask, bytes, index;
> char* bin;
>
> mask = 1;
> bytes = 1;
> index = 0;
> bin = malloc(bytes);

Be sure to check that malloc() succeeded.
Even a request for a single byte can fail.

>
> do
> {
> if(n & mask)
> bin[index++] = '1';
> else
> bin[index++] = '0';
> mask *= 2;
> bin = realloc(bin, ++bytes);

This is just waiting for trouble.
If realloc() fails, it returns a NULL pointer, and leaves the old buffer
intact. With the call above, if realloc() fails, the NULL pointer
overwrites the _only_ pointer to the old buffer, so you have created a nice
memory leak.

It is better to do the re-allocation like this:
  char* temp = realloc(bin, ++bytes);
  if (temp != NULL)
  {
    bin = temp;
  }
  else
  {
    free(bin);
    /* Report failure to the calling code */
  }

> } while(mask <= n);
>
> bin = realloc(bin, ++bytes);

Same here.

> bin[index] = '\0';
> bin = revstr(bin); /* make it big-endian */
>
> return bin;
> }

Bart v Ingen Schenau 

-- 
a.c.l.l.c-c++ FAQ: http://www.comeaucomputing.com/learn/faq
c.l.c FAQ: http://www.eskimo.com/~scs/C-faq/top.html
c.l.c++ FAQ: http://www.parashift.com/c++-faq-lite/

Relevant Pages

  • Re: Memory Managers for Beginners 3
    ... also about how the complete virtual memory system works. ... There is no "realloc attempt" happening that fails at all. ... is quite clear what that pointer is used for. ...
    (borland.public.delphi.language.basm)
  • Re: How to free double ** memory
    ... to the realloc, but if realloc modifies a pointer from a malloc, ... You only free *alloc pointers ... If you call reallocwith a null pointer as its first argument, ... If reallocfails, ...
    (comp.lang.c)
  • Re: Simple realloc() question
    ... if it fails. ... The old pointer value is indeterminate if realloc returns ... about comparing the old and new. ...
    (comp.lang.c)
  • Re: Recovering gracefully from a realloc() failure
    ... But this has the side effect of clobbering the "zones" pointer if ... // it worked, update the pointer ... What I was wondering was how realloc() actually behaved. ... old value of "zones" be considered to be valid if reallocfails ...
    (comp.lang.c)
  • Re: realloc() implicit free() ?
    ... >> another] what happens to the input block when realloc() finds it ... > you must instead use the output pointer value. ... The realloc function changes the size of the object pointed to by ... new, deallocate the old, and return a pointer to the new -- but I see ...
    (comp.lang.c)