Thread: Optimize memory management

  1. #1
    Registered User
    Join Date
    Aug 2015
    Posts
    17

    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.

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    27,776
    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.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    Registered User
    Join Date
    Aug 2015
    Posts
    17
    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;
    }
    Last edited by merafiq; 10-03-2015 at 02:25 PM.

  4. #4
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    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.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  5. #5
    Registered User
    Join Date
    Aug 2015
    Posts
    17
    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.

  6. #6
    Registered User
    Join Date
    Sep 2015
    Location
    Australia
    Posts
    63
    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..?

  7. #7
    Lurking whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    9,616
    Quote Originally Posted by JohnGM View Post
    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.
    Last edited by whiteflags; 10-03-2015 at 04:44 PM.

  8. #8
    Registered User talahin's Avatar
    Join Date
    Feb 2015
    Posts
    51
    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

  9. #9
    Registered User
    Join Date
    May 2012
    Location
    Arizona, USA
    Posts
    725
    Quote Originally Posted by talahin View Post
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. memory management
    By hansigarten2 in forum C Programming
    Replies: 10
    Last Post: 08-19-2010, 01:22 PM
  2. Memory management
    By chris.r in forum C Programming
    Replies: 5
    Last Post: 06-08-2010, 07:14 PM
  3. Memory management in C
    By nicolas in forum C Programming
    Replies: 2
    Last Post: 03-19-2004, 03:25 PM
  4. Memory management
    By CompiledMonkey in forum C++ Programming
    Replies: 9
    Last Post: 12-19-2003, 11:41 AM
  5. memory management...
    By master5001 in forum Game Programming
    Replies: 24
    Last Post: 01-07-2002, 05:50 PM