Optimize memory management

Hi,
With concern of buffer overflow, memory leaks and/or having invalid pointer issue, change of pointer to garbage etc.

I didn't use realloc & strcat to avoid such problem in this program.

Would like to know from experts, how well the below program work on memory management?

Code:

---

int main()
{
    FILE *indexfile;
    indexfile = fopen("supermarket.osm", "a");
    char *file_header="<osm version=\"0.6\" generator=\"osmconvert 0.8.3\">\n";
    char *first_line = "<node id=\"\" lat=\"\" lon=\"\">\n";
    char *second_line = "<tag k=\"name\" v=\"\"/>\n";
    char *third_line  = "<tag k=\"addr:city\" v=\"\"/>\n";
    char *fourth_line = "<tag k=\"addr:street\" v=\"\"/>\n";
    char *fifth_line = "<tag k=\"addr:housenumber\" v=\"\"/>\n";
    char *last_line = "</node>\n";
   
    int node_id=1;
    char *tagvl = "nah und frisch";
    char *cityvl = "Schönberg";
    char *add_street = "Große Mühlenstraße";
    double lat = 54.3884, lon = 10.38285;
    char *housenr = "100";
           
    char *spnt_temp_1st_line = NULL;
    char *spnt_temp_2nd_line = NULL;
    char *spnt_temp_3rd_line = NULL;
    char *spnt_temp_4th_line = NULL;
    char *spnt_temp_5th_line = NULL;
    char *spnt_temp_6th_line = NULL;
   
   
    spnt_temp_1st_line = (char *) malloc(17 + sizeof(char*) * (strlen(first_line)));
   
    sprintf(spnt_temp_1st_line," <node id=\"%d\" lat=\"%1.7f\" lon=\"%1.7f\">\n",
                                node_id,lat,lon);
       
    spnt_temp_2nd_line = (char *) malloc(3 +
                        sizeof(char*) * (strlen(second_line) + strlen(tagvl)));
   
    sprintf(spnt_temp_2nd_line,"  <tag k=\"name\" v=\"%s\"/>\n",tagvl);
       
    spnt_temp_3rd_line = (char *) malloc(3 +
                        sizeof(char*) * (strlen(third_line) + strlen(cityvl)));
   
    sprintf(spnt_temp_3rd_line,"  <tag k=\"addr:city\" v=\"%s\"/>\n",cityvl);
       
    spnt_temp_4th_line = (char *) malloc(3 +
                        sizeof(char*) * (strlen(fourth_line) + strlen(add_street)));
   
    sprintf(spnt_temp_4th_line,"  <tag k=\"addr:street\" v=\"%s\"/>\n",add_street);
       
    spnt_temp_5th_line = (char *) malloc(3 +
                        sizeof(char*) * (strlen(fifth_line) + strlen(housenr)));
   
    sprintf(spnt_temp_5th_line,"  <tag k=\"addr:housenumber\" v=\"%s\"/>\n",housenr);
       
    spnt_temp_6th_line = (char *) malloc(2 +
                        sizeof(char*) * (strlen(last_line)));
   
    sprintf(spnt_temp_6th_line," </node>\n");
       
    char *file_string;
   
    file_string = (char *) malloc(1 + sizeof(char*) *(strlen(file_header) +
                                strlen(spnt_temp_1st_line) +
                                strlen(spnt_temp_2nd_line) +
                                strlen(spnt_temp_3rd_line) +
                                strlen(spnt_temp_4th_line) +
                                strlen(spnt_temp_5th_line) +
                                strlen(spnt_temp_6th_line)));
   
    sprintf(file_string,"%s%s%s%s%s%s%s</osm>\n",file_header,
                                        spnt_temp_1st_line,spnt_temp_2nd_line,
                                        spnt_temp_3rd_line,spnt_temp_4th_line,
                                        spnt_temp_5th_line,spnt_temp_6th_line);
                                       
   
    printf("%s",file_string);
   
    fputs(file_string,indexfile);
   
    //free all malloc
   
    free(spnt_temp_1st_line);
    free(spnt_temp_2nd_line);
    free(spnt_temp_3rd_line);
    free(spnt_temp_4th_line);
    free(spnt_temp_5th_line);
    free(spnt_temp_6th_line);
    free(file_string);
   
    return 0;
}

---

Thanks.

A few comments:

• #include <string.h> as you call strlen.
• #include <stdlib.h> as you call malloc and free.
• Do not cast the return value of malloc.
• Avoid magic numbers like 17 by replacing them with named constants.
• When calling malloc with sizeof, you often can avoid writing an incorrect sizeof by making use of the variable name. For example, instead of this, which is certainly wrong because you want sizeof(char) rather than sizeof(char*):
  
  Code:

  ---

  spnt_temp_1st_line = (char *) malloc(17 + sizeof(char*) * (strlen(first_line)));

  ---

  write:
  
  Code:

  ---

  spnt_temp_1st_line = malloc(17 + sizeof(*spnt_temp_1st_line) * (strlen(first_line)));

  ---

  This may still be wrong though: does the magic number 17 take into account the null character? Furthermore, since you are using sizeof, you are not assuming that spnt_temp_1st_line is a pointer to char, since sizeof(char) == 1 is always true. Therefore, you should more correctly write:
  
  Code:

  ---

  spnt_temp_1st_line = malloc(sizeof(*spnt_temp_1st_line) * (17 + strlen(first_line)));

  ---

Quote:

---

Originally Posted by merafiq

With concern of buffer overflow, memory leaks and/or having invalid pointer issue, change of pointer to garbage etc.

I didn't use realloc & strcat to avoid such problem in this program.

---

That's fine, but your approach of having spnt_temp_1st_line, ..., spnt_temp_6th_line looks too cumbersome. You should be able to calculate the amount of space needed and do just one malloc call, then build the string using that one dynamic array. In fact, you are already doing this with file_string.

Hi,
Many thanks for your reply.
I've count alike, lat + lon both taken 8 character = 16 + 1 NULL = 17.
[not sure whether its correct]

Now, I endup as below ->

Code:

---

int main()
{
    FILE *indexfile;
    indexfile = fopen("supermarket.osm", "a");
    char *file_header="<osm version=\"0.6\" generator=\"osmconvert 0.8.3\">\n";
    char *first_line = "<node id=\"\" lat=\"\" lon=\"\">\n";
    char *second_line = "<tag k=\"name\" v=\"\"/>\n";
    char *third_line  = "<tag k=\"addr:city\" v=\"\"/>\n";
    char *fourth_line = "<tag k=\"addr:street\" v=\"\"/>\n";
    char *fifth_line = "<tag k=\"addr:housenumber\" v=\"\"/>\n";
    char *node_close = "</node>\n";
    char *last_line = "</osm>\n";
   
    char *node_id="11111";
    char *tagvl = "nah und frisch";
    char *cityvl = "Schönberg";
    char *add_street = "Große Mühlenstraße";
    double lat = 54.3884, lon = 10.38285;
    char *housenr = "100";
       
    char *spnt_temp_all;
   
    spnt_temp_all = malloc(sizeof(*spnt_temp_all) * (17 + strlen(file_header) +
                                                          strlen(first_line)  +
                                                          strlen(node_id)    +
                                                          strlen(second_line) +
                                                          strlen(tagvl)          +
                                                          strlen(third_line)  +
                                                          strlen(cityvl)      +
                                                          strlen(fourth_line) +
                                                          strlen(add_street)  +
                                                          strlen(fifth_line)  +
                                                          strlen(housenr)      +
                                                          strlen(node_close)  +
                                                          strlen(last_line)));
   
    sprintf(spnt_temp_all,
    "<osm version=\"0.6\" generator=\"osmconvert 0.8.3\">\n"
    " <node id=\"%s\" lat=\"%1.5f\" lon=\"%1.5f\">\n"
    "  <tag k=\"name\" v=\"%s\"/>\n"
    "  <tag k=\"addr:city\" v=\"%s\"/>\n"
    "  <tag k=\"addr:street\" v=\"%s\"/>\n"
    "  <tag k=\"addr:housenumber\" v=\"%s\"/>\n"
    " </node>\n"
    "</osm>\n",node_id,lat,lon,tagvl,cityvl,add_street,housenr);
   
  //  printf("%s",spnt_temp_all);
   
    fputs(spnt_temp_all,indexfile);
    free(spnt_temp_all);
    return 0;
}

---

In all honesty, you should never build xml like that. Always use an XML library.
I've seen far too many bugs caused by people stringing bits of text together and creating broken XML.

Hi,
Thanks for your advise.
But my requirement is quite little [extracting, and generating with only required value] (I'm also concern of broken xml).
Includes very less chance of using library.

Thanks.

Hi

If I may pop in with a question....regarding the castng of malloc......laserlight said..."Do not cast the return value of malloc", where every book, document and the like says you cast to the required type..

Is there a reason you say that.....I had tested it recently and does not seem to make much dfference....soo maybe something behind the scenes going on..?

Quote:

---

Originally Posted by JohnGM

Hi

If I may pop in with a question....regarding the castng of malloc......laserlight said..."Do not cast the return value of malloc", where every book, document and the like says you cast to the required type..

Is there a reason you say that.....I had tested it recently and does not seem to make much dfference....soo maybe something behind the scenes going on..?

---

Problems in other languages notwithstanding...

Books who show casting malloc aren't entirely wrong about it, but they aren't right either. The reason people say not to cast malloc is because it's not really necessary.

When malloc is properly prototyped like this - as in stdlib.h -

Code:

---

void *malloc(size_t bytes);

---

The void pointer type is implicitly convertible to any other data pointer type, by a rule of the Standard.

Like many tools in C, you should use it only if you know what you are doing. If you don't include stdlib.h, but use malloc with a cast, you actually have a bug, and the C compiler would happily go along with what was written. More typically than that, all sub-expressions have a default result type. It's not uncommon to see people cast an expression because it's a means to an end; sometimes people would otherwise get results that they do not expect, like the sign extension that happens going from signed char to unsigned long.

Code:

---

char ch = 0xE2;
printf("%#X\n", ch);

0XFFFFFFE2

---

You could write cast to fix this, but you could also declare ch as an unsigned char. Both will work. Both are (arguably) clear. Casting usually means declaring variable(s) one way and then using them another way, though. It's confusing.

Hi,

as to add to whiteflags comment. Original malloc would return (char *) and casting was needed. If I recall correctly C99 redefined the return from malloc as (void *) and casting is no longer needed. That's why people advise against casting malloc's return value.

Cheers

Quote:

---

Originally Posted by talahin

Hi,

as to add to whiteflags comment. Original malloc would return (char *) and casting was needed. If I recall correctly C99 redefined the return from malloc as (void *) and casting is no longer needed. That's why people advise against casting malloc's return value.

Cheers

---

It was C89 that introduced the change to malloc, but otherwise you're correct.