tags:

views:

69

answers:

4

Hello,

gcc 4.4.4 c89

In main I call a function to pass a line of text to a function. I want to perform some operation on it. However, that would mean that line is of no use. So in my get_string function I copy the contents and return the result. The only problem, is that the memory to that result would be lost and pointing to something unexpected.

I am just wondering how can I pass the result back, without and still keep the ordinal line of data?

Many thanks for any advice,

code snippet from main:

    if(fgets(line_data, (size_t)STRING_SIZE, fp) == NULL) {
        fprintf(stderr, "WARNING: Text error reading file line number [ %d ]\n", i);
    }

    if(get_string(line_data) != NULL) {
        if(strcmp(get_string(line_data), "END") == 0)
            break;
    }
    else {
        fprintf(stderr, "WARNING: Cannot get name of student at line [ %d ]\n", i);
    }

    /* Fill student info */
    strncpy(stud[i].name, line_data, (size_t)STRING_SIZE);

Call this function

char* get_string(char *line_data)
{
    char *quote = NULL;
    char result[STRING_SIZE] = {0};

    strncpy(result, line_data, (size_t)STRING_SIZE);

    /* Find last occurance */
    if((quote = strrchr(result, '"')) == NULL) {
        fprintf(stderr, "Text file incorrectly formatted for this student\n");
        return NULL;
    }
    /* Insert nul in place of the quote */
    *quote = '\0';

    /* Overwite the first quote by shifting 1 place */
    memmove(result - 1, result, strlen(result) + 1);

    return result;
}
+1  A: 

You want to malloc the memory for result:

char *result; result = malloc(STRING_SIZE);

As you have it, the memory for result exists on the stack and thus only during the time that execution is inside get_string()

You'll also need to free result before returning NULL to prevent a memory leak.

Kevin Stock
Not sure, but I always thought it was bad practice to allocate inside a function. I thought it should be the callee that should be responsible for allocating and free memory.
robUK
+1  A: 

For your direct question - either use malloc(3) and tell the user of the function to de-allocate the return pointer (this is sort of prone to memory leaks since it's so easy to ignore return value in C), or provide the second parameter as a receive buffer:

char* get_string( const char* line_data, char* receive_buf, size_t buf_size );

The third parameter is for the function to know how large the receive buffer is.

Now to your code - the line memmove(result - 1, result, strlen(result) + 1); corrupts your stack.

Nikolai N Fetissov
The memmove is there because the get_string will remove the double quotes. i.e. "Low, Lisa" would be returned as just Low, Lisa. I nul terminate the 2nd one, Then overwrite the first one. If you know a better method, I would be happy to know.
robUK
`result-1` is an invalid address. You are overwriting something else on the stack. How about `memove( result, result + 1, strlen( result ));` ?
Nikolai N Fetissov
+2  A: 

Just return strdup(result). It will allocate and copy your string. However, you have to free the result after using it in the outer function.

You also could take a buffer in input (with its size), and fill it with what you want.

Scharron
+1  A: 

As a rule of thumb, you should never return a pointer to a function's local variable. You know why: once a function returns, the memory allocated for its variables can be reused for something else. The idea to return a pointer to the result buffer is inherently bad.

You should think whether you really need to keep a copy of the quoted string. What if you tested the "END" string before calling get_string? If you need to quote and output data later, it is done easily. Say:

printf("\"%s\"", student_record);

So get_string could actually work in the buffer in place and return the error code (0 for success). Since you know the final result is a smaller nul terminated string, you wouldn't even need a length parameter.

int get_string(char* student_record);

If you really need to keep a copy of the quoted string, then you need to pass another buffer. I'd still return an int to indicate success (0) or failure (say -1).

int get_string( const char* line_data, char* student_record, size_t buf_size );

I personally prefer letting the caller allocate its own buffer. It leaves it a chance to use a fixed length buffer (simpler memory management). Ex:

char student_record[512];
...
if (!get_string(student_record)) {
  // error
}
Philippe A.