0

I am writing a very simple struct called Process, and the code seems implemented correctly at a quick glance, but upon testing my code it seems to keep crashing the program, either by sysMalloc assertion failure or a double free() error when one attempts to free it.

Some relevant code:

Declaration

typedef struct Process{
    int pid;
    int background;
    int group;
    int status;
    char* name = 0;
} Process;

Constructor

Process* Process_init(char* name, int pid, int group, int background){
    Process* output    = (Process*)calloc(1, sizeof(Process*));
    output->name       = name;
    output->pid        = pid;
    output->group      = group;
    output->background = background;
    output->status     = 0;

    return output;
}

Calling code

char* command = "python";
char* command1= "cat < testing";

Process* proc = Process_init("command", 1, 1, 1);
Process* proc2 = Process_init("command1", 1, 1, 1);

There is some weird behavior where it appears to work the first time around, but causes the sysMalloc error when called a second time (which is why I call it twice.)

I tried using Valgrind and it gave me the following:

invalid write of size 4
==3485==    at 0x8049A03: Process_init (process.c:6)
==3485==    by 0x80488C2: main (test.c:58)
==3485==  Address 0x419e730 is 12 bytes after a block of size 4 alloc'd
==3485==    at 0x402425F: calloc (vg_replace_malloc.c:467)
==3485==    by 0x80499F9: Process_init (process.c:5)
==3485==    by 0x80488C2: main (test.c:58)
==3485== 
==3485== Invalid write of size 4
==3485==    at 0x8049A14: Process_init (process.c:8)
==3485==    by 0x80488C2: main (test.c:58)
==3485==  Address 0x419e728 is 4 bytes after a block of size 4 alloc'd
==3485==    at 0x402425F: calloc (vg_replace_malloc.c:467)
==3485==    by 0x80499F9: Process_init (process.c:5)
==3485==    by 0x80488C2: main (test.c:58)
==3485== 
==3485== Invalid write of size 4
==3485==    at 0x8049A1D: Process_init (process.c:9)
==3485==    by 0x80488C2: main (test.c:58)
==3485==  Address 0x419e724 is 0 bytes after a block of size 4 alloc'd
==3485==    at 0x402425F: calloc (vg_replace_malloc.c:467)
==3485==    by 0x80499F9: Process_init (process.c:5)
==3485==    by 0x80488C2: main (test.c:58)
==3485== 
==3485== Invalid write of size 4
==3485==    at 0x8049A23: Process_init (process.c:10)
==3485==    by 0x80488C2: main (test.c:58)
==3485==  Address 0x419e72c is 8 bytes after a block of size 4 alloc'd
==3485==    at 0x402425F: calloc (vg_replace_malloc.c:467)
==3485==    by 0x80499F9: Process_init (process.c:5)
==3485==    by 0x80488C2: main (test.c:58)
==3485== 
==3485== Invalid write of size 4
==3485==    at 0x8049A03: Process_init (process.c:6)
==3485==    by 0x80488EA: main (test.c:59)
==3485==  Address 0x419e768 is 12 bytes after a block of size 4 alloc'd
==3485==    at 0x402425F: calloc (vg_replace_malloc.c:467)
==3485==    by 0x80499F9: Process_init (process.c:5)
==3485==    by 0x80488EA: main (test.c:59)
==3485== 
==3485== Invalid write of size 4
==3485==    at 0x8049A14: Process_init (process.c:8)
==3485==    by 0x80488EA: main (test.c:59)
==3485==  Address 0x419e760 is 4 bytes after a block of size 4 alloc'd
==3485==    at 0x402425F: calloc (vg_replace_malloc.c:467)
==3485==    by 0x80499F9: Process_init (process.c:5)
==3485==    by 0x80488EA: main (test.c:59)
==3485== 
==3485== Invalid write of size 4
==3485==    at 0x8049A1D: Process_init (process.c:9)
==3485==    by 0x80488EA: main (test.c:59)
==3485==  Address 0x419e75c is 0 bytes after a block of size 4 alloc'd
==3485==    at 0x402425F: calloc (vg_replace_malloc.c:467)
==3485==    by 0x80499F9: Process_init (process.c:5)
==3485==    by 0x80488EA: main (test.c:59)
==3485== 
==3485== Invalid write of size 4
==3485==    at 0x8049A23: Process_init (process.c:10)
==3485==    by 0x80488EA: main (test.c:59)
==3485==  Address 0x419e764 is 8 bytes after a block of size 4 alloc'd
==3485==    at 0x402425F: calloc (vg_replace_malloc.c:467)
==3485==    by 0x80499F9: Process_init (process.c:5)
==3485==    by 0x80488EA: main (test.c:59)
--Snip--

So, it looks like the problem is in the constructor, but I'm not exactly sure why that is since it looks like a really straightforward moving around of variables.

Jonathon Reinhart
  • 132,704
  • 33
  • 254
  • 328
Saxophlutist
  • 293
  • 5
  • 21
  • chante the line `Process* output = (Process*)calloc(1, sizeof(Process*));` to `Process* output = (Process*)calloc(1, sizeof(Process));` – Andro May 31 '14 at 20:36

1 Answers1

3
Process* output    = (Process*)calloc(1, sizeof(Process*));

This is incorrect. You're only allocating enough memory for a pointer (4 or 8 bytes).

The proper code:

Process* output    = calloc(1, sizeof(*output));

Improvements:

  1. It's now correct! We are allocating enough bytes for whatever output points to, which is a Process, instead of enough bytes for just a pointer to Process.
  2. By saying sizeof(*output), we remove the additional reference to the type name (DRY), which ensures your code is correct, even if someone changes the type that output points to, and forgets to change the rest of the line.
  3. I've removed the cast from the result of calloc. calloc returns void* which can be assigned to any pointer type variable without a cast.

Also, you should be checking the return value of any function, specifically one that returns memory:

Process* Process_init(char* name, int pid, int group, int background){
    Process* output    = calloc(1, sizeof(*output));
    if (output == NULL)
        return NULL;

    output->name       = name;
    output->pid        = pid;
    output->group      = group;
    output->background = background;
    output->status     = 0;

    return output;
}

Any consumer of this function should check its return value as well.

Community
  • 1
  • 1
Jonathon Reinhart
  • 132,704
  • 33
  • 254
  • 328
  • Excellent. It works completely. Excellent point on sizeof(*output) instead of using the typename. I'm going to have to start using that from now on. – Saxophlutist Jun 01 '14 at 20:32