Thread: Weird realloc issue

Your inner realloc is using the wrong strings.

*str is a short string, so if you have ABC%DEF you'll allocate enough for ABC and the argument. You're buffer overrunning when you get to DEF

This code keeps track of
- the length of the format
- the length of ALL the arguments already added to the string.

If you want to be pedantic, the amount of memory allocated is over what is needed by the number of % characters in the format string.

Code:

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

int my_sprintf(char **str, char *format, ...);

int main() {
    char *sql_string=NULL;
    char name[]="John Doe";
    char people[]="Company";

    my_sprintf(&sql_string,"SELECT '%' FROM '%'",name,people);
    printf("%s\n",sql_string);

    free(sql_string);
    return 0;
}

int my_sprintf(char **str, char *format, ...) {
    va_list ap;
    char *next_arg;
    size_t  formatLen = strlen(format); // length of format
    size_t  allArgLen = 0;  // length of ALL arguments added so far

    va_start(ap, format);

    //Allocate space for "SELECT '%' FROM '%'"
    (*str)=realloc((*str),(formatLen+1)*sizeof(char));
    (*str)[0] = '\0';

    //i is the position in the first string
    //j is the position in the entire output string including arguments that are inserted in place of %'s
    //k is the position in each of the arguments that will be replacing the %'s
    int i=0;
    int j=0;
    for (i=0;i<formatLen;i++) {
        if (format[i]=='%') {
            //The first run will allocate space for "SELECT 'John Doe' FROM '%'"
            //The second run will allocate space for "SELECT 'John Doe' FROM 'Company'"
            next_arg=va_arg(ap, char *);
            size_t  nextArgLen = strlen(next_arg);
            (*str)=realloc((*str),(formatLen+allArgLen+nextArgLen+1)*sizeof(char));
            for (int k=0;k<nextArgLen;k++) {
                (*str)[j++]=next_arg[k];
            }
            allArgLen += nextArgLen;
        }
        else {
            //For all other non-% characters, write the next character from the first string into str
            (*str)[j++]=format[i];
        }
    }
    (*str)[j]='\0';  // no longer strlen this at all, so we can \0 at the end
    va_end(ap);

    return 0;
}



$ gcc -std=c99 -Wall -g foo.c
$ valgrind ./a.out
==2974== Memcheck, a memory error detector
==2974== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==2974== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==2974== Command: ./a.out
==2974== 
SELECT 'John Doe' FROM 'Company'
==2974== 
==2974== HEAP SUMMARY:
==2974==     in use at exit: 0 bytes in 0 blocks
==2974==   total heap usage: 3 allocs, 3 frees, 83 bytes allocated
==2974== 
==2974== All heap blocks were freed -- no leaks are possible
==2974== 
==2974== For counts of detected and suppressed errors, rerun with: -v
==2974== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)

Oh, and for extra bonus points, the realloc should always be done as

Code:

void *temp = realloc( p, size );
if ( temp != NULL ) {
  p = temp;
} else {
  // panic!
  free( p );
}

Trashing your actual pointer results in a memory leak if realloc happens to fail.

Originally Posted by jtay

Basically what I'm trying to do is write my own version of sprintf that will dynamically allocate memory for the output string based on the length of the input strings. The purpose for this is to make building SQL statements easier; I don't want to have to worry about how long some of the table name, and other values are and allocate unnecessarily large spaces for each statement.

Minimizing mem usage is a good thing, but not when taken to this extreme, because what you will really be doing with this is wasting a lot of processor cycles on nothing.

I suppose SQL statements and table names can be of any length, but realistically, one statement is unlikely to be more than 1kB long, and certainly less than 4kB unless you are completely deranged.

The system allocates memory in "page" size blocks, on most desktops 4096 bytes. It also probably gives you default stack space well in excess of a single page, more like megabytes, most of which is unused.

Ie, the idea that you can shave a few hundred or even a few thousand bytes off your mem usage is false. Unless you know there are going to be tables with paragraph long names, just use a 1kB local stack buffer. If you are really worried, you could check the arg lengths first and return an error -- the point at which your SQL queries have hit 1024 characters might be the point at which you should consider why that is necessary at all.

Originally Posted by jtay

I realize that allocating a large buffer size for each string and calling it a day might have been the best option for this project.

Just to re-iterate, 1000 or 2000 or 4000 characters is larger than a tweet, but don't consider it "a large buffer size". 4kB is a small amount of memory.

I just got so attached to the whole dynamic reallocation thing. Plus, sometime in the future I might try and write my text game again, in which case there will be paragraph sized entries in table fields (for room descriptions), so this will be a useful bit of code for me.

Ah, for adding data. Good point, silly me. However, you still don't have to do this, unless you want just one function for all possible statements -- inserts, selects, etc -- which is a bit silly. Consider:

Code:

int SQLinsertText (char *table, char *field, char *data);  // prototype
char data[] = "blah blah blah as much as you like";
SQLinsertText("some_table", "some_field", data);

If you need to concat those strings inside the function for a call to your SQL API, you can just use strlen to get the total size of the arguments and malloc a buffer for that. I think you will find this much simpler; there is no need to spend a lot of time on unnecessary complications