C : Segmentation Fault when move main function to new file

Question

I implement a custom memory allocator. All my main code for this inside file memory.c I create a main function in this file to test function. All works fine. But when I move those testing code to another file (calling main.c and run it. I meet Segmentation fault.

int main (int argc, char *argv []) {
   allocator_init();
   char* ptr = (char*) allocate(4096);   // csutom function. that on `memory.c` 
   strcpy(ptr, "this is the test one");// segmentation fault here
   printf("print: %s\n", ptr);
   deallocate(ptr);
}

Here is the main code :

volatile Memory memory;


/* allocated memory to memory variable by assign /dev/zero to memory */
void allocator_init() {
    fd = open("/dev/zero", O_RDWR);
    if(fd == -1) {
        perror("File open failed");
        exit(0);
    }

    // page size can different on different platform. customize again to optimize
    PAGE_SIZE = getPageSize();

    // fd = open("ZEROES", O_RDWR);

    if(fd == -1) {
        perror("File open failed");
        exit(0);
    }

    // Initialize the region list
    memory.region = NULL;

    int i;
    /// Initialize the caches
    /// size of each cache is 16 * 2^i => 16, 32, 64, 128, 256, 512, 1024, 2048
    for (i=0; i<8; i++) {
        memory.cache[i].size = 16<<i;
        memory.cache[i].S = NULL;
    }
    return;
}

    void *allocate_region (unsigned int size) {

      Region *region, *temp;

      temp = memory.region; 
      void *mapped_addr = mmap(NULL, size + REGION_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);

      if(mapped_addr == MAP_FAILED) {
         perror("Mapping failed");
         exit(0);
       }

      /* create region from mapped address */
      region = mapped_addr;
      region->size = size;
      region->addr = mapped_addr + REGION_SIZE;
      printf("allocated region: %p\n", region->addr);

      /* assign this region to memory */
      memory.region = region;

      /* just simple append algorithm in linked list. so new region will be appended to head of linked list */
      region->next = temp;

      return region->addr;

    }

/* allocate : if size < 2048 : allocate cache. else allocate region */
void *allocate(unsigned int size) {
    size = ALIGN(size);
    return allocate_region(size);
}

Here is memory.h define all struct that i have used:

#ifndef MEMORY_H
#define MEMORY_H

#define MAX_SIZE (1024*1024)
#define REGION_SIZE sizeof(Region)
#define SLAB_SIZE sizeof(Slab)
#define WORD_SIZE 32
#define ALIGNMENT 8
/* rounds up to the nearest multiple of ALIGNMENT */
#define ALIGN(size) (((size) + (ALIGNMENT-1)) & ~0x7)
#define SIZE_T_SIZE (ALIGN(sizeof(size_t)))
#define TRUE 1
#define FALSE 0
#define MAX_SIZE 10000

// Caches are:
// 16, 32, 64, 128, 256, 512, 1024, 2048


#include "bits.h"

int PAGE_SIZE;
// File descriptor for zeroes file
int fd;

void allocator_init();
int deallocate_cache(void* ptr);
void *allocate(unsigned int size);


typedef struct __Region {
  void *addr;               /// started address can use for caller (can calc by allocated address + Region size)
  int size;                 /// size of this allocated Region (not include size Region)
  struct __Region *next;
} Region;

/// size of Slab equals to size of System Page
typedef struct __Slab {
  void *addr;               /// address of this slab (exclude header)
  char *bitmap;             /// started address can use for caller (can calc by allocated address + slab size)
  int size;
  int slots;                /// number of slots in maximum has been used
  int cache_size;            /// size of cache that contains this slab
  int bitmap_size;          /// size of bitmap. so, can count address bit by bitmap + bitmap_size
  int currentSize;          /// current allocated elements of this slab. currentSize = 0. deallocated this slab
  int bit;                  /// bit to marked which part of slab has been used
  struct __Slab * next;
} Slab;



typedef struct __Cache {
  int size;
  Slab *S;
} Cache;


typedef struct __Memory {
  Region *region;
  Cache cache[8];
} Memory;
#endif // MEMORY_H

above code will works fine, allocate function return an address. I just meet error when move to another file. in my memory.c, I have some global variable to control allocated address and memory. Does it affect when I move code to new file ? I cannot explain this.

Thanks :)

Answer 1

There are some observations worth making:

1. Your header should only declare what the consumer of your code needs to know. That's three functions: allocator_init(), allocate() and deallocate_cache(). Everything else in the file is an implementation detail that should be hidden from the consumer of your code.
2. Headers should not define variables — not even tentatively. If you aren't writing extern in front of a variable in a header, it should be prefixed with static and you'd better have a very good reason for it. (See How do I share a variable between source files in C for a complete disquisition.)
3. Names starting with _ are reserved for the implementation to use. That's a slight over-simplification, but it covers all the bases and is easier to remember than the formally correct rules. Names starting with __ are definitely off-limits (100%; no simplification involved). Don't define such names for yourself, and be extremely cautious about using such names in your code (they're usually types you probably shouldn't be using). I simply removed the leading underscores from the structure tags; it works happily.
4. Your header included "bits.h", but fortunately your code didn't use anything from it, so commenting it out worked fine.
5. Your code uses getPageSize() but doesn't define it. Probably excusable; I used 4096 instead. And tracking this down showed that PAGE_SIZE is a variable defined in the header, leading to the start of this diatribe polemic explaining what to to. It should be a static variable in the memory.c file. The only things visible from the outside of the source file should be those functions (and perhaps global variables) that consumers need to use. Everything else should be static so it is invisible — no name conflicts, no possibility of misuse or abuse.
6. When it is made into a static variable, the compiler complains that PAGE_SIZE is unused. That, incidentally, is another advantage of making everything that can be static into a static definition — when the code could be accessed outside the source because it is not static, the compiler can't warn about unused variables or functions.
7. Your header defined two different values for MAX_SIZE; don't!
8. Your header defined unused values, such as #define TRUE 1; again, don't. (MAX_SIZE wasn't used, either, nor FALSE, nor SLAB_SIZE, …)

9. Your ALIGN macro is faulty:

    #define ALIGN(size) (((size) + (ALIGNMENT - 1)) & ~0x7)
   

   As it stands, ALIGNMENT is 8, and the macro works. But suppose I change ALIGNMENT to 16; then the mask is wrong. It should probably be & ~(ALIGNMENT - 1). Beware of apparent parameterization that is incomplete.

10. If you exit because of an error, you should not exit with status 0; that means success. Either use 1 or EXIT_FAILURE — or follow some other locally relevant scheme.
11. Your header declared deallocate_cache() but your main program calls deallocate() and the code for neither was given. The naming inconsistency is problematic. I used a dummy implementation.

Obviously, the necessary headers had to be added, too. OK; with that lot fixed, the code compiled and linked. Then I ran into:

Mapping failed: Operation not supported by device

This was Mac OS X 10.9.2 Mavericks. Just an FYI. I worked around it by creating an empty file ./dev.zero and referencing that. Beware portability assumptions.

Then it crashed with a bus error, signal 10. It didn't print the allocated region message. That limits the damage to 3 lines of code.

You cannot do arithmetic on void * types in standard C. GCC does allow you to do it; don't take advantage of that, though, in code that has any pretensions to portability.

When I created an empty file, the program crashed. When I used dd if=/dev/zero of=dev.zero bs=1k count=1024 to initialize the file to all zeros, the program no longer crashed. I'd added a bunch of debug printing code.

I suggest not using /dev/zero for your mapping file.

Mapping succeeded: 0x10ca74000
region = 0x10ca74000
size = 4096
allocated region: 0x10ca74018
Memory: allocate_region (0x10ca40080)
Region: Base (0x10ca74000)
Address: 0x10ca74018, size:   4096, next = 0x0
Cache: line 0 (0x10ca40088)
Size: 16
Cache: line 1 (0x10ca40098)
Size: 32
Cache: line 2 (0x10ca400a8)
Size: 64
Cache: line 3 (0x10ca400b8)
Size: 128
Cache: line 4 (0x10ca400c8)
Size: 256
Cache: line 5 (0x10ca400d8)
Size: 512
Cache: line 6 (0x10ca400e8)
Size: 1024
Cache: line 7 (0x10ca400f8)
Size: 2048
ptr = 0x10ca74018
print: this is the test one
deallocate called for 0x10ca74018: unimplemented

Code

memory.h

#ifndef MEMORY_H
#define MEMORY_H

extern void  allocator_init(void);
extern void  deallocate(void *ptr);
extern void *allocate(unsigned int size);

#endif // MEMORY_H

main.c

#include <stdio.h>
#include <string.h>
#include "memory.h"

int main(void)
{
    allocator_init();
    char *ptr = (char *) allocate(4096); // custom function. that on `memory.c`
    printf("ptr = %p\n", ptr);
    strcpy(ptr, "this is the test one"); // segmentation fault here
    printf("print: %s\n", ptr);
    deallocate(ptr);
}

memory.c

#include "memory.h"
#include <assert.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <unistd.h>

#define REGION_SIZE sizeof(Region)

#define ALIGNMENT   8
#define ALIGN(size) (((size) + (ALIGNMENT - 1)) & ~(ALIGNMENT - 1))

#if defined(DEV_ZERO_MEMORY_MAPPING)
#define DEV_ZERO "/dev/zero"
#else
#define DEV_ZERO "./dev.zero"
#endif

enum { NUM_CACHES = 8 };

static int fd = -1;

typedef struct Region
{
    void *addr;
    int size;
    struct Region *next;
} Region;

typedef struct Slab
{
    void *addr;
    char *bitmap;
    int size;
    int slots;
    int cache_size;
    int bitmap_size;
    int currentSize;
    int bit;
    struct Slab *next;
} Slab;

typedef struct Cache
{
    int size;
    Slab *S;
} Cache;

typedef struct Memory
{
    Region *region;
    Cache cache[NUM_CACHES];
} Memory;

static Memory memory = { 0 };

static void dump_slab(FILE *fp, const char *tag, const Slab *slab)
{
    fprintf(fp, "Slab: %s (%p)\n", tag, slab);
    if (slab != 0)
    {
        fprintf(fp, "addr: %p, ", slab->addr);
        fprintf(fp, "bitmap: %p, ", (void *)slab->bitmap);
        fprintf(fp, "size: %6d, slots %3d, ", slab->size, slab->slots);
        /*
        int cache_size;
        int bitmap_size;
        int currentSize;
        int bit;
        struct Slab *next;
        */
    }
}

static void dump_cache(FILE *fp, const char *tag, const Cache *cache)
{
    fprintf(fp, "Cache: %s (%p)\n", tag, cache);
    if (cache != 0)
    {
        fprintf(fp, "Size: %d\n", cache->size);
        Slab *slab = cache->S;
        while (slab != 0)
        {
            dump_slab(fp, "", slab);
            slab = slab->next;
        }
    }
}

static void dump_region(FILE *fp, const char *tag, const Region *reg)
{
    fprintf(fp, "Region: %s (%p)\n", tag, reg);
    if (reg != 0)
    {
        fprintf(fp, "Address: %p, size: %6d, next = %p\n",
                reg->addr, reg->size, reg->next);
    }
}

static void dump_memory(FILE *fp, const char *tag, const Memory *mem)
{
    fprintf(fp, "Memory: %s (%p)\n", tag, mem);
    if (mem != 0)
    {
        Region *reg = mem->region;
        dump_region(fp, "Base", reg);
        while (reg->next != 0)
        {
            dump_region(fp, "Next", reg->next);
            reg = reg->next;
        }
        for (int i = 0; i < NUM_CACHES; i++)
        {
            char line[32];
            snprintf(line, sizeof(line), "line %d", i);
            dump_cache(fp, line, &memory.cache[i]);
        }
    }
}

void allocator_init(void)
{
    fd = open(DEV_ZERO, O_RDWR|O_CREAT, 0600);
    if (fd == -1)
    {
        perror("File open failed");
        exit(0);
    }

    if (fd == -1)
    {
        perror("File open failed");
        exit(0);
    }

    memory.region = NULL;

    for (int i = 0; i < NUM_CACHES; i++)
    {
        memory.cache[i].size = 16 << i;
        memory.cache[i].S = NULL;
    }
}

static void *allocate_region(unsigned int size)
{
    assert(fd != -1);

    Region *temp = memory.region;
    void *mapped_addr = mmap(NULL, size + REGION_SIZE, PROT_READ | PROT_WRITE,
                             MAP_PRIVATE, fd, 0);

    if (mapped_addr == MAP_FAILED)
    {
        perror("Mapping failed");
        exit(0);
    }
    printf("Mapping succeeded: %p\n", mapped_addr);

    Region *region = mapped_addr;
    printf("region = %p\n", region);
    region->size = size;
    printf("size = %d\n", region->size);
    region->addr = (char *)mapped_addr + REGION_SIZE;
    printf("allocated region: %p\n", region->addr);

    memory.region = region;
    region->next = temp;

    dump_memory(stderr, __func__, &memory);

    return region->addr;
}

void *allocate(unsigned int size)
{
    size = ALIGN(size);
    return allocate_region(size);
}

void deallocate(void *ptr)
{
    fprintf(stderr, "%s called for %p: unimplemented\n", __func__, ptr);
}

I strongly recommend making functions to dump complex structures along the dump_memory(), dump_region(), dump_cache(), dump_slab() functions shown; they're often very helpful, though they were actually something of a red herring in debugging this.