3

I have the following code that has been working for the last couple of months, but have recently started to crash on occasion (when running in a multi-threaded application):

struct some_struct {
    char* m_str1;
    char* m_str2;
}

struct some_struct*
set_some_struct(const char* p_str1, const char* p_str2) {
    struct some_struct* some_struct_ptr = 
        (struct some_struct*)malloc(sizeof(struct some_struct));
    if (some_struct_ptr == NULL)
        printf("malloc failed!\n");

    size_t str1_len = strlen(p_str1) + 1;
    size_t str2_len = strlen(p_str2) + 1;

    some_struct_ptr->m_str1 = malloc(str1_len);
    if (some_struct_ptr->m_str1 == NULL)
        printf("malloc failed!\n");

    some_struct_ptr->m_str2 = malloc(str2_len); // Crashes here
    if (some_struct_ptr->m_str2 == NULL)
        printf("malloc failed!\n");

    strcpy(some_struct_ptr->m_str1, p_str1);
    strcpy(some_struct_ptr->m_str2, p_str2);

    return some_struct_ptr;
}

Running it gives me "The instruction at "0x7c81bb52" referenced memory at "0x00000002". The memory could not be "read"."

Is there anything obviously wrong with the code above that could have it misbehave under certain circumstances? If I run the function alone in a test program it works just fine, but it always crashes when running in the full application. Everything leading up to the third malloc seems just fine.

EDIT: Further investigation leads me to believe that it is earlier calls to malloc that mess this one up. Is such a thing even possible? If I uncomment a function call being made previous to set_some_struct and that involve several mallocs then set_some_struct will run just fine.

  • If it compiles that's not the problem but you are missing semicolon after mallocs – David Ranieri Jan 17 '13 at 10:37
  • 1
    Note the flow of the code is unchanged if the initial allocation fails (i.e. for `some_struct_ptr`), it just prints a failure message. Meaning, potentially, a `NULL` pointer is being dereferenced. Remove the cast from return value of `malloc()` and ensure `#include ` is present. – hmjd Jan 17 '13 at 10:43
  • Maybe your program just runs out of memory. Do an `abort()` after printing the `malloc failed', so execution of your program doesn't continue in this case. Also print the error message to stderr, and call fflush(stderr) afterwards just to make sure the message is written. – pts Jan 17 '13 at 10:45

2 Answers2

2

Well, all you do when an allocation fails is print an error; perhaps the printing is dropped or you're missing it? If there are multiple threads running this, output might be confusing.

Second, you're not checking the input pointers. Since the crash is a read, and all other accesses through pointers are writes to your newly allocated area(s), I suspect one or more of the arguments is a NULL pointer. You should check for that.

Also, you shouldn't be casting the return value of malloc() in C (see here for reasons), if you're not including stdlib.h this could be hiding errors.

If the strings are constant, you could save memory and speed by doing just a single call to malloc(), adding up the sizes of the three allocations first and then setting pointers accordingly, of course.

Community
  • 1
  • 1
unwind
  • 391,730
  • 64
  • 469
  • 606
  • I'm pretty sure I'm not missing output. If I insert prints before and after the third call to malloc the one before will show up just fine. And stdlib.h is included. Cool tip on doing it all with just one malloc though! – user1986698 Jan 17 '13 at 11:06
1
if (some_struct_ptr == NULL)
    printf("malloc failed!\n");

From this point on you're using a garbage pointer. The same problem occurs with the following code.

if (some_struct_ptr->m_str1 == NULL)
    printf("malloc failed!\n");

if (some_struct_ptr->m_str2 == NULL)
    printf("malloc failed!\n");
JimR
  • 15,513
  • 2
  • 20
  • 26