the basics of malloc

Thanks again for everyone's input. I dug around and found the old code from which this originates. Mind you, this works but it doesn't feel right and I'm not sure of one particular part. I stripped the code to keep it small for posting.

First, here is a data file named ttt.dat

05/16/08 22:31 aaaa
05/16/08 22:32 bbb
05/16/08 22:33 asdfasdf

Each line needs to be manipulated so I read each line as one string and then start manipulation. Here is the code:

Code:

---

#include <stdio.h>
#include <string.h>
#include <malloc.h>

#define NumberOfNotes 25      /* The number of notes the program will allow    */

typedef struct {                        /* structure of note        */
        char nBody[20];                /* text of note                */
        char nExtra[300];                /* area for extra info such as date string        */
} NOTEINFO;

NOTEINFO *note[NumberOfNotes] ;

main ()
{
        FILE *dataStream ;
        int ctr ;                                                /* utility counter */
        int finis = 0 ;                                /* boolen for testing */
        char noteContent[300] = "" ;                /* string for manipulation of file data */

        for( ctr = 0; ctr < NumberOfNotes; ctr++ )        /* loop through number of notes */
        {        note[ctr] = (NOTEINFO *)malloc( sizeof( NOTEINFO ) ) ;        /* allocate memory */
                if( note[ctr] == NULL )
                {        printf( "Error allocating." ) ;        /* print error */
                        finis = 1 ;
                }
        }

        dataStream = fopen( "ttt.dat", "r" ) ;                /* open file for reading */
        if( dataStream == NULL )                                /* test opening */
        {        printf( "Error opening file." ) ;                /* print error message */
                finis = 1 ;
        }

        ctr = 0;
        while( !finis )
        {
                if( fgets( noteContent, 300, dataStream ) == NULL )        /* read the string */
                        finis = 1 ;                                        /* set boolean for end of read */
                else
                {        strncpy( note[ctr]->nExtra, noteContent, 14 ) ;        /* copy first 14 chars to nExtra */
                        printf( note[ctr]->nExtra ) ;                /* print nExtra */
                        printf( "\n" ) ;
                        strncpy( note[ctr]->nBody, &noteContent[19], strlen( noteContent) - 1 ) ;
                        printf( note[ctr]->nBody ) ;                /* print nBody */
                }
                ctr++ ;
        }
        fclose( dataStream );

        for( ctr = 0; ctr < NumberOfNotes; ctr++ )        /* loop through all note variables */
                free( note[ctr] ) ;                                        /* free the memory */

        return( 0 ) ;
}

---

First I'm confused with the strncpy(). It is used twice: first to grab the date info via:

Code:

---

strncpy( note[ctr]->nExtra, noteContent, 14 ) ;

---

And then for the note body:

Code:

---

strncpy( note[ctr]->nBody, &noteContent[19], strlen( noteContent) - 1 ) ;

---

As I understand, the second param to strncpy is a string pointer. Why do I need the "&" in the second strncpy but not the first?

Second, the whole things just looks sloppy. I'm open for suggestions on cleaner code.

Thanks in advance.

Dale L

You could have written the first as
strncpy( note[ctr]->nExtra, &noteContent[0], 14 ) ;


> #include <malloc.h>
Must be very old code then. The place where allocation functions are prototyped is stdlib.h

> note[ctr] = (NOTEINFO *)malloc( sizeof( NOTEINFO ) ) ;
In ANSI-C, there is no need for the cast. Though if you're still compiling this old code on an old compiler, it might still be.

There is actually a bug in the way allocated memory is used.
The strncpy may not actually copy a \0 to make the result a proper C string, and malloc doesn't guarantee to clear memory either. So the result may be you see some (perhaps quite a lot) of random characters at the end of your line.


> printf( note[ctr]->nExtra ) ;
NEVER do this - that is pass user input as the format string to printf.
http://en.wikipedia.org/wiki/Format_string_attack

Thanks for the info Salem. Wish I had known about the malloc situation years ago. I can remember pulling my hair out over the unexpected characters.

I'm thinking now (beware!)

What if you took an old C program that uses malloc and rename it to a cpp file, then you take the mallocs and use "new" instead? If memory serves, some of the header files could also be removed.

It would seem that doing so would clean up the allocation and string problems. Do you know?

Thoughts welcome.

Dale L

New is identical to malloc, but it takes into account C++'s new features, such as classes, and therefore also constructors and destructors. Otherwise there's little difference between the two.
So in itself, it wouldn't help anything. RAII could help with allocation/deallocation, but that's a different subject.

Agreed. (But then I DID warn you when I started thinking.)

To ensure all comes up clean I suppose following the malloc with a memset would work.

Dale L

Or you could use calloc.

Quote:

---

Originally Posted by robwhit

Or you could use calloc.

---

Yes, it's probably more efficient than a combination of malloc() and memset(), if nothing else because it's only one call in your code so it makes the code a tiny bit shorter.

--
Mats