• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 411
  • Last Modified:

code review

This piece of code creates a linklist (it tries at least :), now I like to know what modifications I should do , what parts are good, if it's dangourus? and etc. The only thing that can not be chanaged is the structure of my function "create_node", i.e its parameters and return settings (because that would mean alot of extra work for me to change my application). If you suggest modification please use standard POSIX-system functions (strcmp(), fork()). I want this code to be as safe and solid as possible.


#include <stdlib.h>

struct node {
    char str2[50];
    struct node *next;
};

struct node* create_node(const char* str);
     main()
     {
           struct node *root = create_node("hi");
           struct node *tmp = root;
           int i = 0;
           for (i = 0; i < 2; i++) {
           tmp->next = create_node("hello");
           tmp = tmp->next;
      }

       //here we have to free the memory
       free(tmp);
       free(root);
 }

//create a new struct and connect to the linklist
struct node* create_node(const char* str)
{
struct node *pos=malloc( sizeof(struct node) );
strcpy(pos->str2,str);
pos->next=NULL;
return pos;
}
0
Thunder_scream
Asked:
Thunder_scream
  • 13
  • 10
  • 7
  • +3
4 Solutions
 
Dragon_KromeCommented:
in function create_node:
you don't check if str is NULL.
you don't check if malloc fails (returns NULL).
strcpy is unsafe, you can overflow the 50 chars if str is larger than that - use strncpy
0
 
efnCommented:
Your program allocates three nodes and only frees two.  It would be better to have a deallocation function that traverses the whole list, freeing every node.
0
 
Dragon_KromeCommented:
Use valgrind to detect memory allocation and other problems:
this is the output from valgrind with your program:


valgrind --tool=memcheck --leak-check=full --show-reachable=yes ./a.out
==8196== Memcheck, a memory error detector.
==8196== Copyright (C) 2002-2005, and GNU GPL'd, by Julian Seward et al.
==8196== Using LibVEX rev 1575, a library for dynamic binary translation.
==8196== Copyright (C) 2004-2005, and GNU GPL'd, by OpenWorks LLP.
==8196== Using valgrind-3.1.1, a dynamic binary instrumentation framework.
==8196== Copyright (C) 2000-2005, and GNU GPL'd, by Julian Seward et al.
==8196== For more details, rerun with: -v
[ SNIP ]
==8196==
==8196== 56 bytes in 1 blocks are definitely lost in loss record 1 of 1
==8196==    at 0x4021650: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==8196==    by 0x8048498: create_node (a.c:27)
==8196==    by 0x8048458: main (a.c:15)
==8196==
==8196== LEAK SUMMARY:
==8196==    definitely lost: 56 bytes in 1 blocks.
==8196==      possibly lost: 0 bytes in 0 blocks.
==8196==    still reachable: 0 bytes in 0 blocks.
==8196==         suppressed: 0 bytes in 0 blocks.
0
VIDEO: THE CONCERTO CLOUD FOR HEALTHCARE

Modern healthcare requires a modern cloud. View this brief video to understand how the Concerto Cloud for Healthcare can help your organization.

 
PaulCaswellCommented:
Hi Thunder_scream,

// This is ok :-)
#include <stdlib.h>

// This will break on some compilers. You should use a 'typedef' for true portability and therefore quality.
struct node {
    char str2[50];
    struct node *next;
};

// You should put a comment here with a brief description of what it does. If you are evere searching for the parameters to create_node this is the first reference you will easily recognise.
struct node* create_node(const char* str);

// Better to declare 'main' properly. Switch on all warnings in your compiler and fix them all. You will find many holes this way.
     main()
     {

...

// Proper layout is also a boon to tracking down potential bugs.
           for (i = 0; i < 2; i++) {

...

I agree with the others on the issues they've raised.

Paul
0
 
efnCommented:
> // This will break on some compilers. You should use a 'typedef' for true portability and therefore quality.
> struct node {

This looks like pretty basic, standard C to me.  Paul, can you identify any compiler that won't compile a structure tag, or will compile it incorrectly?
0
 
jitendra_wadhwaniCommented:
Everyone has given right suggestions but

efn has given most inportant suggestion.....


Program has Memory Leaks....


0
 
PaulCaswellCommented:
efn,

>>Paul, can you identify any compiler that won't compile a structure tag, or will compile it incorrectly?
No, I cant. My comment was less than accurate. Nowadays it is usual to declare a type using a typedef, not a structure tag. Sorry.

Paul
0
 
Dragon_KromeCommented:
> efn has given most inportant suggestion.....
> Program has Memory Leaks....

@jitendra_wadhwani :

A leak is just a leak. The worst thing that could happen is memory exhaustion. On the other hand, a buffer overflow is a much more serious security issue, which can lead, under proper conditions, to priviledge escalation and similar problems. There are *lots* of articles and books on this matter, for example:

http://www.securityfocus.com/infocus/1596
http://securecoding.org/
http://www.cert.org/books/secure-coding/

just to metion a few..
0
 
jitendra_wadhwaniCommented:
This is a very small program where we are passing some hasrcoded values....

Each program should be secure and there should be proper checkpoints for lot of things....

For 3 nodes there is nothing to going happen wrong....nither buffer overflow nor memory leaks...

But if you are trying to implement anything implement it properly...


It depends upon you code and your view what is much dangerous...
 
At My point of view memory leaks is thre most important issue....for you it may be something else....

>>There are *lots* of articles and books on this matter

 There are *lots* of articles and books on EVERY MATTER.....

Doesnt mean you are going to read each and everything.......

>>just to metion a few..

Mentioning something is very easy....

Ask yourself how many you have read out of . ONLY THESE MENTIONED....

Good Day
Jitendra
0
 
Thunder_screamAuthor Commented:
Thanks for the suggestions I will try to modify the code accordingly and post again later today perhaps
0
 
Dragon_KromeCommented:
> For 3 nodes there is nothing to going happen wrong....nither buffer overflow nor memory leaks...
> It depends upon you code and your view what is much dangerous...
> At My point of view memory leaks is thre most important issue....for you it may be something else....

right, with this exact program, no buffer overflow will happen.

but what if, let's say, we take the name of the root node from argv[1] ?

drk@nevermore ~ $ ./test hi
drk@nevermore ~ $

Everything is ok, besides that leak.

Now what if we stuff more that 50 chars in that name?

drk@nevermore ~ $ ./test `perl -e 'print "A" x 100'`
*** glibc detected *** ./test: free(): invalid next size (fast): 0x0804a088 ***
Aborted (core dumped)
drk@nevermore ~ $

Ups.

Yeah, you can say i've modified the original program to allow "custom" input data - just a POC; in real world you have to be careful with everything, yes, including the leak. But some programming errors can lead to system compromise and others just eat memory. If you can crash the program using the memory leak, i'll accept it has the same importance as the buffer overflow :)

0
 
jitendra_wadhwaniCommented:
what if??
This question covers the whole world.........

===

If your program eats memory...finally it will be crashed but before that crash......You and your system both have to suffer...

Before crashing your system will be dead(untill it crashes and os releaves teh memory used with your app)......
You can not switch to another app....

Another app will


In case of overflow crash your application will be crashed...no body else has to suffer...

This depend upon nature of program.....even you can say nature of programmer...

What is the most important for you?
Can be programmed that I should be alive doesn't matter how much neighbours and society have to suffer?

I think memory leak is the most important thing...I know I am not going to change your view...

IN NUTSHELL.........

Overflow?
Only Your application will fail......Will crash soon
But you know where it can fail as you are the programmer you know from where I have passed arguments...


Memory leaks ?
Your application will fails.....
But System will be almost dead when it is about to crash.....
Chances for crash will increase as when you program will require new memory...may be is will not get and that will become te reason of application failure...

>>i'll accept it has the same importance as the buffer overflow :)
Doesn't matter, you accept it or not....Again saying the same thing depends upon nature of program....which is important...


0
 
jitendra_wadhwaniCommented:
>>Now what if we stuff more that 50 chars in that name?

Every beginner uses that hardcoded size....here same Thunder_scream has done....

This question willl arise for any size...

dynamically allocation is the solution to this problem which every programmer learns and uses with experience..

Dragon_Krome,
Sorry If my words have hurt you....
0
 
Dragon_KromeCommented:
Ok, let me explain this to you:

1) We have a leak, in *THIS* program.
This leak is likely to disappear if you will use the code in another program, since the leak itself is in the test program of create_node.

2) We have a buffer overflow, which will manifest in *ANY* code which will use create_node.


> If your program eats memory...finally it will be crashed but before that crash......You and your system both have to suffer...

This makes me think you're a Windows or ms-dos programmer, in Linux the VM will kill the process if it allocates TOO much memory. No system will crash.

> In case of overflow crash your application will be crashed...no body else has to suffer...
You have no idea about secure programming, do you? http://en.wikipedia.org/wiki/Buffer_overflow 


> Every beginner uses that hardcoded size....here same Thunder_scream has done....
> This question willl arise for any size...
> dynamically allocation is the solution to this problem which every programmer learns and uses with experience..

This is bullshit, you're telling me that if the user wants to feed you a 1Mb string , you'll allocate the whole size? Come on :)))

> Dragon_Krome,
> Sorry If my words have hurt you....

Don't worry, i can handle your wisedom :)
0
 
Dragon_KromeCommented:
A, and if you think this is about the points, it's not. If Thunder_Scream wants to grant me the points, please give them to jitendra_wadhwani  :)
0
 
Dragon_KromeCommented:
..or the others, sorry :)
0
 
jitendra_wadhwaniCommented:
@Dragon_Krome
It is clear from the statments from you
My words have really hurt Dragon_Krome ......

>>You have no idea about secure programming, do you
Writing this line required just 1-2 seconds
You can imagine what ideas I have and how much I know?

>>This makes me think you're a Windows or ms-dos programmer,
Yes I am...

>>in Linux the VM will kill the process if it allocates TOO much memory. No system will crash.
Your Conclusion : Memory leak is not important, Isn't it?


No I cant chage your views or mind set....because you are burning right now.....

>>A, and if you think this is about the points, it's not. If Thunder_Scream wants to grant me the points, please give them to jitendra_wadhwani  :)

This proves you are burning...


Do you think my first comment.............

"Everyone has given right suggestions but

efn has given most inportant suggestion.....


Program has Memory Leaks...."

.............was written for Points?



>>This is bullshit, you're telling me that if the user wants to feed you a 1Mb string , you'll allocate the whole size? Come on :)))

Already told you that everythign is hardcoded....and everthing depends nature of program....

See the program ...Where user is going to feed anything....

1MB?  Your programs nature can be such a way that it wont accept string more that n chara....
here if 50 is hardcoded can be limited to 50...


And BOSS, who am I to tell you more....What ever told earlier that seems to be enough..repeating again
1. Objective of program...
2. What if?.......covers the entire world...



>>Don't worry, i can handle your wisedom :)
You should be always capable of that....May god help you


You are trying to make is words-war.....

EE is not for personal words-war.....so not going to add more comments to this question......

I know although you might be thinking to write somethign wrong to abobe line and this line might stop you....Anyway Your comments are welcome and I will be reading your comments...
0
 
Dragon_KromeCommented:
I apologize the discution went offtopic.
0
 
Thunder_screamAuthor Commented:
I had a bad feeling about this code to start with, now I know why:) I use to c a great while ago so I'm kind of rosty.

so the record should be changed like this then ..

typedef struct{
    char str2;
    struct node *next;
}node;

or

typedef struct{
    char str2[50];
    struct node *next;
}node;

I will work on the body now ...
0
 
Thunder_screamAuthor Commented:
ok I have updated the code ....

#include <stdlib.h>
#include <string.h>

typedef struct node {
    char str2[50];
    struct node *next;
}NODE;

//forwarding fn
struct node* create_node(const char* str);

void main()
{
      NODE *root = create_node("hi");
      if(root!=NULL){
            NODE *tmp = root;
        int i = 0;
        for (i = 0; i < 2; i++) {
              tmp->next = create_node("hello");
              tmp = tmp->next;
            }
            free(tmp);
    }


      //here we have to free the memory .... its only a sketch...not sure about this though
       NODE *temp=root;
       while(temp->next!=NULL){
          NODE *next = temp->next;
          free( temp);
            temp=next;
      }
      free(root);
}
//create a new struct and connect to the linklist
struct node* create_node(const char* str)
{

      NODE *pos=(NODE*)malloc( sizeof(NODE) );
      if(str!=NULL){
            strncpy(pos->str2,str,50);
            pos->next=NULL;
      }
      return pos;
}

please remember that I want this code to be rock solid.
0
 
efnCommented:
Style suggestion:  use either "NODE" or "struct node" consistently, not a mixture of both.

It's not a good idea to free the third node in the list while the second node is still pointing to it.  (free(tmp);)

Near the top of main, it checks for root being null, and if it is null, it doesn't add to the list.  However, farther down, it assigns root to temp and then tries to dereference temp.  If root was null, this is likely to lead to an unhappy ending.

The loop at the end of main fails to free the last node, for which temp->next == NULL.  It frees root twice, though, once in the loop, and again after it.
0
 
Dragon_KromeCommented:
Nope, still not good:

1)
     NODE *pos=(NODE*)malloc( sizeof(NODE) );
     /*
      You should check for str != NULL at the beginning of the function, then after malloc for pos != NULL
      */
     if(str!=NULL){    
          /* It could be easier to change that 50 to a define to allow easier changing, or you can use sizeof(pos->str2)
              WARNING: strncpy  does NOT NULL terminate your string if the size of copied data is >= sizeof buffer, therefore
              you HAVE TO make sure you place a '\0' at the end of your str2 (str2[49]) manually, after strncpy
          */
          strncpy(pos->str2,str,50);
          pos->next=NULL;
     }
     return pos;

2) Also, change the return type of main to int to conform to the standards.

3) In main():

     NODE *root = create_node("hi");
     if(root!=NULL){
     ...
    }

     What if this fails ( root == NULL )? You use root a below, you'll get a crash.

      for (i = 0; i < 2; i++) {
             tmp->next = create_node("hello");        /* What if this returns NULL ? */
             tmp = tmp->next;
          }
         free(tmp);         /* Why do you free this? You're deleting your root node! */

     //here we have to free the memory .... its only a sketch...not sure about this though
      NODE *temp=root;    /* root is already free()d via free(tmp) */
      while(temp->next!=NULL){
         NODE *next = temp->next;  /* this could not work, you're in unsafe ground, in free()d memory */
         free( temp);          /* double free() - very bad! */
          temp=next;
     }

Relevant valgrind output:

==9667== Invalid read of size 4
==9667==    at 0x8048486: main (test.c:28)
==9667==  Address 0x417612C is 52 bytes inside a block of size 56 free'd
==9667==    at 0x40227F8: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==9667==    by 0x804847C: main (test.c:22)
==9667==
==9667== Invalid free() / delete / delete[]
==9667==    at 0x40227F8: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==9667==    by 0x80484B2: main (test.c:33)
==9667==  Address 0x4176028 is 0 bytes inside a block of size 56 free'd
==9667==    at 0x40227F8: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==9667==    by 0x804849F: main (test.c:30)

Basically it confirms the free() problems.
0
 
lhbiltwCommented:
When you call malloc here, it looks very ugly:

NODE *pos=(NODE*)malloc( sizeof(NODE) );
if(str!=NULL){
    strncpy(pos->str2,str,50);
    pos->next=NULL;
}

This is not the usual procedure to test for a NULL return value. If malloc has returned NULL, this is a serious error (you're out of heap space) and your program just ignores it and goes on with its business. What's worse, you return a null pointer to the rest of the program, so that the calling code needs to test for it again. A better approach for most applications is:

NODE *pos = (NODE*)malloc(sizeof(NODE));
if (str == NULL) {
    fprintf(stderr, "Out of memory\n");
    exit(-1);
}

After memory allocation has failed, there's not much hope of continuing execution, it shouldn't be attempted unless your program really needs to save some vital data before it shuts down... and you would have to be careful in doing that to avoid doing anything that needs another heap allocation. Another case where you might not want to abort the program is if your program has allocated some memory that it might be able to free in order to make space, but not many applications keep around data structures that they don't really need.

Another serious problem is that you have used strncpy to avoid a buffer overflow when copying your string, but you haven't checked the result of that call to make sure that the string in fact fit into the allowed space. This is important because, if str is longer than 50 bytes, then the copy will be 50 bytes long and will not be terminated by a null character. This means that an attempt to copy or print it will then read past the end of its buffer and spit out garbage or crash your porgram (or cause other undefined behavior).

For example:

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

int main(int argc, char **argv)
{
      char * src = "Hello!";
      char dst[4];
      strncpy(dst,src,4);
      puts(dst);
}

On my system this program prints different garbage each time it runs.

The strncpy manual page: http://www.die.net/doc/linux/man/man3/strncpy.3.html

Since strncpy is guaranteed to fill in the rest of the destination string with null bytes after it reaches the null terminator of the source string, you can test to see if the source fit into the destination by looking at the last byte (pos->str2[49] in your example) to see if it's null. If not, you could either resort to some kind of error handling, or patch things up by truncating the string to the size that's available like this:

if (pos->str2[49] != '\0')
    pos->str2[49] = '\0';

I would recommend that instead of manipulating the linked list directly when adding and removing nodes, you define functions that are dedicated to those operations and then call them whenever you need to change the list. This will avoid many errors down the line. One good way to free a linked list is with a recursive function:

void free_list(NODE * list) {
    if (NODE == NULL)
        return;
    free_list(list->next);
    free(list);
}

Linked lists and recursive calls are a natural fit because the data structure itself is recursively defined.

As a final comment, usually fixed-length string buffers are used to avoid allocating heap space, or else to guarantee that all chunks of data are the same length so you can make contiguous arrays that can be accessed quickly. You are using a linked list so the latter doesn't apply, and you are already using malloc so the former doesn't apply. You might as well use malloc to allocate the string's storage space as well and they you will be guaranteed to never overflow its buffer.
0
 
Dragon_KromeCommented:
lhbiltw,

>NODE *pos = (NODE*)malloc(sizeof(NODE));
>if (str == NULL) {
>    fprintf(stderr, "Out of memory\n");
>   exit(-1);
>}

it's  "if ( pos == NULL )... ". check my last comment.

>if (pos->str2[49] != '\0')
>  pos->str2[49] = '\0';

There's no need for this check, you can simply place \0 at the str2[49] position, because it's more efficient (size, and perhaps speed, depends on the compiler): in your case you have a read access at str2[49] and in the worst case you have a write access too.

The idea of using functions is good. In your example free_list is ok, but not recursively! It has no advantage in this case - what if you're free()ing a list of a million elements? You'll call free_list a million times 'till you actually get to release something. An iterative approach is more suitable, even though it might not be as intuitive as recurisivity.
0
 
lhbiltwCommented:
>it's  "if ( pos == NULL )... ".

Yes, a typo on my part.

>There's no need for this check, you can simply place \0 at the str2[49] position,
>because it's more efficient (size, and perhaps speed, depends on the compiler):
>in your case you have a read access at str2[49] and in the worst case you have
>a write access too.

You're right, I missed the comment in your code about strncpy in your previous comment, it's better to just write '\0' as you said, IF you plan to just truncate the string. By checking for success, though (and one might not have realized the method for doing it as it's not spelled out clearly in the documentation), you could substitute some other behavior to handle overflow in case it's a big deal for you not to discard part of your data. (My name often gets truncated by a 20-character name field in corporate databases, maybe such code could send an error message to developers telling them that it's time to upgrade to a larger field?)

> The idea of using functions is good. In your example free_list is ok, but not recursively!

As for the recursion, if you're using a million-item list then you have a much bigger problem than a recursive algorithm, which is a million non-consecutive memory hits every time you walk the list, plus a million mallocs and a million frees upon creation and deletion, this gets quite expensive. A linked list is the wrong choice of storage for a list that size if you're worried about efficiency. By choosing a linked list as storage, then, you have already chosen to sacrifice some efficiency for flexibility.

You might improve on the linked list for large cases by allocating one huge buffer for your list and growing it sequentially in the buffer, if you're doing that then maybe it's worth worrying about whether your function is iterative or recursive. (After building the initial list in this way you could still add, insert and delete nodes as much as you wanted.) The problem with this approach would be that it wastes memory if you're going to be deleting lots of nodes, since the gaps in the big buffer will not be reused until the whole list is freed.

If you only plan to add/remove from one end of the list rather than changing things in the middle, which is common for singly-linked lists, then you might as well implement some kind of array-based stack that grows when it overflows its storage space, you'll get the same functionality as the linked list without the overhead.

Maybe it's worth noting that his choice of a fixed-size buffer in each list element leaves open the possibility using this big-buffer approach easily, since the size of the buffer needed would be calculated by sizeof(NODE)*listSize. If you're gonna do it that way though, you can ask yourself whether you really want to use a linked list or whether you can live with the inflexibility of array storage.

Anyway, your argument against recursion only applies to the way that C code works in theory, since most compilers will actually generate much better code. Rearrange the recursive function a bit to make it a proper tail call:

void freeList(NODE * list)
{
      if (list == NULL)
            return;
      NODE * nxt = list->next;
      free(list);
      freeList(nxt);
}

Then turn on the compiler's optimization setting, and it generates iterative code. On my system, with gcc -O1, this generates the same code as its iterative cousin. In fact, with the same compiler options, gcc recognized that the version in my previous post could be optimized into a loop and appears to have generated code that's even MORE efficient than the iterative version; I don't know x86 assembler well enough to really judge if it's more efficient, but appears to be using constant stack space at least, and it uses fewer instructions. I agree, though, that the recursive solution has no general advantage here, and the iterative code has the advantage that it's more efficient with optimizations turned off, if you care.
0
 
Thunder_screamAuthor Commented:
ok I did some further modifications... please be very critcal I dont mind..


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


typedef struct node {
    char *str2;
    struct node *next;
}NODE;

//forwarding fn
NODE* create_node(const char* str);
char * dup_str (char *str);
void printlist(NODE *head);
void free_list(NODE * list);

int main()
{
      NODE *root = create_node("Root of linklist ");
      if(root!=NULL){
            NODE *tmp = root;
        int i = 0;
        for (i = 0; i < 2; i++) {
              tmp->next = create_node("Nodes of LL ");
              tmp = tmp->next;
            }
    }
      else
      {
            fprintf(stderr, "Something Bad happend\n");
            free_list(root);
          exit(-1);
      }

      printlist(root);
      free_list(root);
}

//here we have to free the memory
void free_list(NODE * list)
{
      if (list == NULL)
        return;
    NODE * nxt = list->next;
    free(list);
    free_list(nxt);
}


//create a new struct and connect to the linklist
NODE* create_node(const char* str)
{
      NODE *pos=NULL;
      if(str!=NULL)
      {
            pos=(NODE*)malloc(sizeof(NODE));
            if(pos != NULL){
                  pos->str2=dup_str(str);
                  pos->next=NULL;
            }
      }
      else
      {
            fprintf(stderr, "Out of memory\n");
          exit(-1);
      }

      return pos;
}

//make new string
char * dup_str (char *str)
{
      size_t len = strlen(str);
      char *result = malloc(len+1);
      if(result!=NULL)
      {
               strcpy(result, str);
      }
      else
      {
            fprintf(stderr, "Out of memory\n");
          exit(-1);
      }
         return result;
}

//print the list
void printlist(NODE *head)
{
      NODE *prtmp=head;
      while(prtmp!=NULL)
      {
            printf("LL contains text %s ",prtmp->str2);
            prtmp=prtmp->next;
      }
}

I skipped the fixed array and tried to use a dynamic string function.
and also I'm abit skeptical about the heap out of memory error handling I have done,
maybe I should return all errors from all fucntions to the main and output one error msg there? (somehow)

Cheers!
0
 
Dragon_KromeCommented:
1 ) In main():

     else
     {
          fprintf(stderr, "Something Bad happend\n");
          free_list(root);
         exit(-1);
     }

On this branch you'll be if the root node wasn't allocated, therefore you don't need free_list(root)

2) In main():

    if(root!=NULL){
          NODE *tmp = root;
        int i = 0;      

You don't really need to initialize i here, since you initialize it in for; though, it's a good practice to initialize vars when declared, if possible, so you won't forget about  some of them. Be aware though, that some C compilers will complain about  int i not being declared at the beginning of the function.


3)

NODE* create_node(const char* str)
{
     NODE *pos=NULL;
     if(str!=NULL)
     {
          pos=(NODE*)malloc(sizeof(NODE));
          if(pos != NULL){
               pos->str2=dup_str(str);
               pos->next=NULL;
          }
     }
     else
     {
          fprintf(stderr, "Out of memory\n");
         exit(-1);
     }

     return pos;
}


You didn't really got it here.  The if (str != NULL) check is just a safety measure to make sure somebody doesn't give you a NULL pointer as the name (this could happen if the name comes from another function) - so there's no "Out of memory" issue there (at least not in your function ;) ).  
Also, for the clarity of the code, it is better to negate the condition and do like this:

if (str == NULL)
{
       return NULL;  /* Or exit(), if you preffer. But note that a NULL pointer here would indicate a logical error somewhere in the program and you might want to spit out some debug message, or test the pointer with assert(str != NULL); on debug */
}

Another issue: This function create_node has weird behaviour: if you give it a NULL pointer as parameter, it will terminate the program via exit(). On the other hand, if allocating a new node fails, it will return pos (which is NULL).

4) You dynamically allocate memory for str2, but i don't see you free()ing it in free_list:
Your free_list shoud look like this:

void free_list(NODE * list)
{
     if (list == NULL)
        return;
    NODE * nxt = list->next;
    free(list->str2);        // <--- ADD THIS
    free(list);
    free_list(nxt);
}

5) In printlist(), add some \n to that printf, so you can properly read the output :P

Other changes I'd do:
* the recursive free_list, but that's up to your liking.
* choose if you return NULL on error or exit the program. Doing both is confusing.
* as i commented before, it is better to choose your if's conditions in such a manner that would produce smaller, more readable code blocks. In main(), for example, when checking the allocation of the root node, you  say  ( root != NULL ) { do_something; };  now, this do_something can grow, enlarging that code block; the other branch will contain only a few instructions, so it is better to say:

if ( root == NULL )
{
       fprintf(stderr,....);
       exit(-1);
}
// And you don't even need an else block now !
// do something here.

0
 
Thunder_screamAuthor Commented:
thanks for the quick response Dragon_Krome

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

typedef struct node {
    char *str2;
    struct node *next;
}NODE;

//forwarding fn
NODE* create_node(const char* str);
char * dup_str (char *str);
void printlist(NODE *head);
void free_list(NODE * list);

int main()
{
      NODE *root = create_node("Root of linklist ");
      if(root==NULL){
            fprintf(stderr, "Out of memory probably\n");
                  exit(-1);
      }
      NODE *tmp = root;
    for (int i = 0; i < 5; i++) {
          tmp->next = create_node("Nodes of LL ");
          tmp = tmp->next;
      }
      printlist(root);
      free_list(root);
}

//here we have to free the memory
void free_list(NODE * list)
{
   if (list == NULL)
        return;
    NODE * nxt = list->next;
    free(list->str2);
    free(list);
    free_list(nxt);
}


//create a new struct and connect to the linklist
NODE* create_node(const char* str)
{
      NODE *pos=NULL;
      if(str==NULL)
            return NULL;
      pos=(NODE*)malloc(sizeof(NODE));
      if(pos != NULL){
            pos->str2=dup_str(str);
            pos->next=NULL;
      }
      return pos;
}

//make new string
char * dup_str (char *str)
{
      size_t len = strlen(str);
      char *result = malloc(len+1);
      if(result==NULL)
            return NULL;
      else
            strcpy(result, str);
      return result;
}

//print the list
void printlist(NODE *head)
{
      NODE *prtmp=head;
      while(prtmp!=NULL)
      {
            printf("LL contains text: %s\n",prtmp->str2);
            prtmp=prtmp->next;
      }
}

so here we go again...is it better now?, i dont really mind the recursion. Other than that am I doing well in valgrind? no memory leaks?
Should I perhaps change the way I populate the linklist? I.E change the for loop?

Cheers!
0
 
PaulCaswellCommented:
Thats much better! :-)

Now some tuning:

     if(str==NULL)
          return NULL;
     pos=(NODE*)malloc(sizeof(NODE));
     if(pos != NULL){
          pos->str2=dup_str(str);
          pos->next=NULL;
     }
 
If the string is null you are refusing to create a node. Is this correct for ypur algorithm? There are many places where you assume that this function does not return null.

   if (list == NULL)
        return;
    NODE * nxt = list->next;

You can only do this in C++. You do this elsewhere too.

I guess the others will notice more but you're getting there! :-)

Paul
0
 
lhbiltwCommented:
One trick that I often use is to replace calls to malloc with something like this:

void * my_malloc(size_t bytes)
{
    void * buf = malloc(bytes);
    if (buf == NULL) {
        fprintf("Out of memory\n");
        exit(-1);
    }
    return buf;
}

Then, just call this function instead of malloc and you never have to worry again about checking the return value of malloc. (Good compiler optimization will inline this function so it loses no efficiency.)
0
 
lhbiltwCommented:
Except that I forgot an argument, the call to fprintf should be:

fprintf(stderr, "Out of memory\n");

Sorry!
0
 
Dragon_KromeCommented:
Your program seems to be ok now and there seem to be no leaks.

==7808== malloc/free: in use at exit: 0 bytes in 0 blocks.
==7808== malloc/free: 12 allocs, 12 frees, 131 bytes allocated.
==7808== For counts of detected errors, rerun with: -v
==7808== All heap blocks were freed -- no leaks are possible.


however :)) there are still a couple of things:
if you want to be ANSI-C compliant (i.e. your program will compile on any ANSI-C compiler), you need to do a couple of modifications:

* Change the C++ style comments to C style  /*  */
*     for (int i = 0; i < 5; i++) {
         tmp->next = create_node("Nodes of LL ");
 
    Declaring int i here isn't valid, move it to the top of the function and use for (i=0; i<5.... ) here

* add a const to dup_str, like   char* dup_str(const char* str)

0
 
Thunder_screamAuthor Commented:
#include <stdlib.h>
#include <stdio.h>
#include <string.h>


typedef struct node {
    char *str2;
    struct node *next;
}NODE;

//forwarding fn
NODE* create_node(const char* str);
char * dup_str (const char *str);
void printlist(NODE *head);
void free_list(NODE * list);

int main()
{
      NODE *root = create_node("Root of linklist ");
      if(root==NULL){
            fprintf(stderr, "Out of memory probably\n");
          exit(-1);
      }
      NODE *tmp = root;
    int i = 0;
      for (i = 0; i < 5; i++) {
          tmp->next = create_node("Nodes of LL ");
          tmp = tmp->next;
      }
      printlist(root);
      free_list(root);
}

//here we have to free the memory
void free_list(NODE * list)
{
      if (list == NULL)
        return;                                          ----> is this c++ , it works in my compiler though? what should I do instead?
    NODE * nxt = list->next;
    free(list->str2);
    free(list);
    free_list(nxt);
}


//create a new struct and connect to the linklist
//If the string is null we wont create a node.
NODE* create_node(const char* str)
{
      NODE *pos=NULL;
      if(str==NULL)
            return NULL;
      pos=(NODE*)malloc(sizeof(NODE));
      if(pos != NULL){
            pos->str2=dup_str(str);
            pos->next=NULL;
      }
      return pos;
}

//make new string
char * dup_str (const char *str)
{
      size_t len = strlen(str);
      char *result = malloc(len+1);
      if(result==NULL)
            return NULL;
      else
            strcpy(result, str);
      return result;
}

//print the list
void printlist(NODE *head)
{
      NODE *prtmp=head;
      while(prtmp!=NULL)
      {
            printf("LL contains text: %s\n",prtmp->str2);
            prtmp=prtmp->next;
      }
}


/*An alternative approach to handle out of heap/memory....I like it but havent used it though
void * my_malloc(size_t bytes)
{
    void * buf = malloc(bytes);
    if (buf == NULL) {
        fprintf(stderr, "Out of memory\n");
        exit(-1);
    }
    return buf;
}
*/


First off thanks for the reminder Sonnycoder, you just proved to me that I was fair( more or less ) e.g these guys deserve a Straight A.
I just want a final comment on the C++ syntax I'm using, its funny it works with my c (only) compiler.(or maybe not)
And I would also use this as an oppertunity to thank you guys for the help.
0
 
Thunder_screamAuthor Commented:
sorry, the sonnycoder comment should be in another thread.

Cheers!
0
 
lhbiltwCommented:
The issue with this code:

void free_list(NODE * list)
{
     if (list == NULL)
        return;                                          ----> is this c++ , it works in my compiler though? what should I do instead?
    NODE * nxt = list->next;      <---- THIS is the statement that violates the ANSI C standard
    free(list->str2);
    free(list);
    free_list(nxt);
}

is that in the ANSI C standard, all variable declarations must come at the top of the function, before statements. Mixing declarations and statements is allowed in C++. It's not just C++, though, the C99 standard also allows mixing, and most C compilers will allow it unless you tell them to strictly follow the old ANSI standard. (The ANSI standard should be superseded by C99 in time, but many compilers don't quite conform to C99 yet.) This is only a concern if your program will be compiled on many different systems. If you're using gcc then you can check for ANSI compliance with the "-ansi" command-line switch, I don't know how other compilers do this.

This is also an issue with your one-line comments (using "//"), those are allowed in C++ and C99 but not in ANSI C.
0
 
Thunder_screamAuthor Commented:
oki doki ...I think I'm done.
I've changed the commenting and the structure alittle bit to follow the ANSI C if you still see c99 code, let me know.

Cheers!


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


typedef struct node {
    char *str2;
    struct node *next;
}NODE;

/*forwarding functions*/
NODE* create_node(const char* str);
char * dup_str (const char *str);
void printlist(NODE *head);
void free_list(NODE * list);

/*Main function creates the list and etc*/
int main()
{
      NODE *root = NULL;            
      NODE *tmp = NULL;                                    
      root=create_node("Root of linklist ");
      if(root==NULL){
            fprintf(stderr, "Out of memory probably\n");
          exit(-1);
      }      
      tmp = root;
        int i = 0;
      for (i = 0; i < 5; i++) {
          tmp->next = create_node("Nodes of LL ");
          tmp = tmp->next;
      }
      printlist(root);
      free_list(root);
}

/*here we have to free the memory*/
void free_list(NODE * list)
{
      NODE * nxt = NULL;
      if (list == NULL)
        return;    
      nxt=list->next;
      free(list->str2);
    free(list);
    free_list(nxt);
}


/*create a new struct and connect to the linklist
If the string is null we wont create a node.*/
NODE* create_node(const char* str)
{
      NODE *pos=NULL;
      if(str==NULL)
            return NULL;
      pos=(NODE*)malloc(sizeof(NODE));
      if(pos != NULL){
            pos->str2=dup_str(str);
            pos->next=NULL;
      }
      return pos;
}

/*make new string*/
char * dup_str (const char *str)
{
      size_t len = strlen(str);
      char *result = malloc(len+1);
      if(result==NULL)
            return NULL;
      else
            strcpy(result, str);
      return result;
}

/*print the list*/
void printlist(NODE *head)
{
      NODE *prtmp=NULL;
      prtmp=head;
      while(prtmp!=NULL)
      {
            printf("LL contains text: %s\n",prtmp->str2);
            prtmp=prtmp->next;
      }
}


/*An alternative approach to handle out of heap/memory
void * my_malloc(size_t bytes)
{
    void * buf = NULL;
      buf=malloc(bytes);
    if (buf == NULL) {
        fprintf(stderr, "Out of memory\n");
        exit(-1);
    }
    return buf;
}
*/
0
 
efnCommented:
> oki doki ...I think I'm done.

You underestimate the experts' ability to pick nits.

For C89 compatibility, you should move the declaration of i up with the other declarations in the main block.

It would be better to check for create_node returning NULL after both calls to it, not just one.

With ANSI/ISO standard C, you don't need to cast the pointer to void returned from malloc.

http://www.lysator.liu.se/c/c-faq/c-3.html#3-5

But you can if you want to maintain compatibility with pre-standard compilers.

It is possible for create_node to create a node with a null str2 pointer, but printlist does not check for this condition.  Either create_node should guarantee that if it returns a non-null pointer, the node does not contain a null str2 pointer, or everything that uses the str2 pointer, such as printlist, should check for a null str2 pointer.

Instead of

     NODE *prtmp=NULL;
     prtmp=head;

you could just write

     NODE *prtmp=head;

Actually, you don't really need prtmp at all.  You could traverse the list using the head pointer.
0
 
lhbiltwCommented:
Personally I thought your code actually looked better before you made it ANSI-compliant. I think we've come to the point where emphasis on compatibility is making you write worse code, and that's not a tradeoff that's worth making IMO. ANSI compliance is overrated in my book, it's really only necessary when you need the absolute widest possible distribution, and from the looks of it, it will be a while before you have enough experience as a C coder to think about distributing your code publicly. If and when you try to compile your program on a different compiler, little compatibility programs might (or might not) arise, I just worry about them when they come up. I would not be surprised if you never have to work with a C compiler that does not support extensions like "//" comments and mixing declarations with statements.
0
 
Dragon_KromeCommented:
I had to move some code written in C once, from gcc to .NET Studio 2003 and to my surprise the bloody compiler was spitting errors at me, until I realised it didn't like my way of declaring variables at any point of the code; gcc was having no problem with this.

That's why I suggested it's better to keep a minimul level of standard compliance. If you think you'll end up compiling your code on various machines with various compilers (like when doing cross-platform programming), it is better to be standard compliant to avoid any problems.

0
 
Thunder_screamAuthor Commented:
#include <stdlib.h>
#include <stdio.h>
#include <string.h>


typedef struct node {
    char *str2;
    struct node *next;
}NODE;

/*forwarding functions*/
NODE* create_node(const char* str);
char * dup_str (const char *str);
void printlist(NODE *head);
void free_list(NODE * list);

/*Main function creates the list and etc*/
int main()
{
      int i = 0;
      NODE *root = NULL;
      NODE *tmp = NULL;
      root=create_node("Root of linklist ");
      if(root==NULL){
            fprintf(stderr, "Out of memory probably\n");
          exit(-1);
      }
      tmp = root;
      for (i = 0; i < 5; i++) {
          tmp->next = create_node("Nodes of LL ");
            if(tmp==NULL){
                  fprintf(stderr, "Out of memory probably\n");
                exit(-1);
            }
          tmp = tmp->next;
      }
      printlist(root);
      free_list(root);
}

/*here we have to free the memory*/
void free_list(NODE * list)
{
      NODE * nxt = NULL;
      if (list == NULL)
        return;
      nxt=list->next;
      free(list->str2);
    free(list);
    free_list(nxt);
}


/*create a new struct and connect to the linklist
If the string is null we wont create a node.*/
NODE* create_node(const char* str)
{
      NODE *pos=NULL;
      if(str==NULL)
            return NULL;
      pos=(NODE*)malloc(sizeof(NODE));
      if(pos != NULL){
            pos->str2=dup_str(str);
            pos->next=NULL;
      }
      return pos;
}

/*make new string*/
char * dup_str (const char *str)
{
      size_t len = strlen(str);
      char *result = malloc(len+1);
      if(result==NULL)
            return NULL;
      else
            strcpy(result, str);
      return result;
}

/*print the list*/
void printlist(NODE *head)
{
      NODE *prtmp=NULL;
      prtmp=head;
      while(prtmp!=NULL)
      {
            printf("LL contains text: %s\n",prtmp->str2);
            prtmp=prtmp->next;
      }
}



/*An alternative approach to handle out of heap/memory
void * my_malloc(size_t bytes)
{
    void * buf = NULL;
      buf=malloc(bytes);
    if (buf == NULL) {
        fprintf(stderr, "Out of memory\n");
        exit(-1);
    }
    return buf;
}
*/

well the only thing that I have changed is the commenting "//" and and initilized my variables with Null or some other appropriate value. Is this not good? I dont understand why you "lhbiltw" think this is worse?

perhaps you mean I have more lines of code now than before?

anyway ...I have learnt a lot from you guys, I have posted one last time with minor modifications..

Cheers!
0
 
lhbiltwCommented:
> I dont understand why you "lhbiltw" think this is worse?
> perhaps you mean I have more lines of code now than before?

Yeah, basically that is the reason. More lines of code are fine if they make your program easier to follow, but I think in this case the extra lines make your program harder to follow. It's a minor inconvenience, though, and since it works and it's compatible, you'd best leave it as you have it.

The outstanding problem now is in the for loop in your main function. You create the node and assign it to tmp->next, but then it is tmp that you check to see if it is null. Note that if tmp were null, your assignment to tmp->next would have already caused a problem since you just dereferenced a null pointer.
0
 
Thunder_screamAuthor Commented:
*Main function creates the list and etc*/
int main()
{
     int i = 0;
     NODE *root = NULL;
     NODE *tmp = NULL;
     root=create_node("Root of linklist ");
     if(root==NULL){
          fprintf(stderr, "Out of memory probably\n");
         exit(-1);
     }
     tmp = root;
     for (i = 0; i < 5; i++) {
         tmp->next = create_node("Nodes of LL ");
          if(tmp->next ==NULL){
               fprintf(stderr, "Out of memory probably\n");
               free_list(root);
               exit(-1);
          }
         tmp = tmp->next;
     }
     printlist(root);
     free_list(root);
}


Yepp you are right, what i meant was as above...does this solve the problem/s ?...

Thanks
0

Featured Post

VIDEO: THE CONCERTO CLOUD FOR HEALTHCARE

Modern healthcare requires a modern cloud. View this brief video to understand how the Concerto Cloud for Healthcare can help your organization.

  • 13
  • 10
  • 7
  • +3
Tackle projects and never again get stuck behind a technical roadblock.
Join Now