Working on key-value disk cache written in plain C. Is there any errors?

I'm an objective-c developer. But i'm getting familar with C. I wrote it for my own develpment in C. Please point on some errors!

The point is to store/read bytes in file named "key" in cachePath directory.

Cache.h

typedef const struct Cache *CacheRef;

/* Returns cache reference for corresponding cache path */
/* NOTE: cache path should exist */

CacheRef CacheAtPath(const char *cachePath);

/* Sets bytes for key */
/* Returns 0 if failed */

int SetBytesForKey(CacheRef cache, const void *bytes, unsigned long length, const char *key);

/* Gets bytes for key */
/* Returns 0 if failed */

int GetBytesForKey(CacheRef cache, void **bytes, unsigned long *length, const char *key);

Cache.c

```
#include "Cache.h"
#include
#include
#include

struct Cache {
char *path;
};

char _FilePathForKey(CacheRef cache, const char key);
FILE _FileForKey(CacheRef cache, const char key, const char *access);

char _FilePathForKey(CacheRef cache, const char key){
size_t pathLength = strlen(cache->path) + strlen(key);
char *path = malloc(pathLength);
strcpy(path, cache->path);
strcat(path, key);

return path;
}

FILE _FileForKey(CacheRef cache, const char key, const char *access)
{
char *filePath = _FilePathForKey(cache, key);
if (filePath == NULL) return NULL;

FILE *file = fopen(filePath, access);

free(filePath);

return file;
}

CacheRef CacheAtPath(const char *cachePath){
struct Cache *cache = malloc(sizeof(struct Cache));
cache->path = malloc(strlen(cachePath));
strcpy(cache->path, cachePath);
return (CacheRef)cache;
}

int SetBytesForKey(CacheRef cache, const void bytes, unsigned long length, const char key)
{
if (cache == NULL) return 0;

FILE *file = _FileForKey(cache, key, "wb");

if (file == NULL) return 0;

int result = (int)fwrite(bytes, length, 1, file);

fclose(file);

return result;
}

int GetBytesForKey(CacheRef cache, void **bytes, unsigned long *length, const char

size_t pathLength = strlen(cache->path) + strlen(key);
char *path = malloc(pathLength);
strcpy(path, cache->path);
strcat(path, key);

Try this instead:

size_t path_len = strlen(cache->path);
size_t key_len = strlen(key);
size_t total_len = path_len + key_len + 1;

char *path = malloc(total_len);
if (!path) { /* TODO: handle error */ }

mempcy(path, cache->path, path_len);
memcpy(path + path_len, key, key_len + 1);

This is better for the following reasons:

-
Your malloc call does not account for the extra 0 byte (also called NUL terminator) that must end C strings. You repeat this mistake in a bunch of other places. (To reiterate: if your string is size len, allocate len + 1 and set the last byte to 0.)

-
strcat will recompute the length of the string at runtime, by searching for that NUL character. This is bad. Every concatenation will lead to another traversal, leading to an O(n2) kind of curve when concatenating lots of strings. I see the use of strcat in pretty much any code as a sort of red flag.

-
malloc can fail. Check your return codes. A large part of writing good C code is being able to answer the question: "how does this code fail?" I feel like a lot of higher-level coders (usually Java and C#, but these days also objC) seem to fail to grasp this and code rather "optimistically" instead.

Error handling in C looks very ugly when done poorly, but if there is a general approach to coding style it can work elegantly.. The trick I like to use is to make your failure paths mostly the same as your success cases. This typically means that there's a part of each function that does "cleanup" - deallocates everything you've allocated, releases whatever's been acquired, etc. Kind of like the RAII pattern in C++ where your locally-declared quantities will do their cleanup in a destructor. This would get run regardless of success or failure. If some allocation or resource acquisition outlives the function, then make the success case be the special case, because this will ultimately allow you to make more fluid code modifications vis-a-vis complex error handling. In case I'm writing too abstractly, an example of this would be:

void *frob(int num_frobs)
{
   void *buf = malloc(/* ... */);
   void *r = buf;   /* Let's say this will be non-null on success cases. */

   if (r && !do_frobbing(buf, num_frobs))
   {
      /* do_frobbing failed! */
      r = NULL;
   }

   if (r && !additional_frobs(buf))
   {
      /* more failure! */
      r = NULL;
   }

   if (r)
   {
      /* success, make sure not to free the buffer on the way out */
      buf = NULL;
   }

   /* clean up*/
   free(buf);

   return r;
}

OK, that's a really goofy-looking example... But here you see that these blocks inside if (r) are really composable. In between each of them you can add and remove more operations that can potentially fail, and your cleanup block handles releasing resources in event of either success or failure. If you're working in C++ this would be done via RAII wrappers, i.e. in destructors of classes and possibly with throw or an early return. But C has no such features to make that easy and correct...

Moving on to other code:

return (CacheRef)cache;

This cast is suspicious. If I read your typedef correctly, all this does is adds an extra const. This means the cast is totally unnecessary. (Another general rule for anal-retentive types such as myself, by the way: casts are suspicious, especially with pointer types. If you can make your code work without casts it's always better.) I would say in general you don't need this typedef.

FILE *_FileForKey(CacheRef cache, const char *key, const char *access)

Technically the C standard says you can't name things with leading underscores. In practice your compiler will let you get away with it anyway. But personally I think it looks weird. If your intention is to hide internal functions from the outside would I suggest you declare them with static and/or add some prefix to their names.

Speaking of names, this brings me to another point... I would work on better and consistent naming conventions. Overall I would strive to have someone be able to read the header, as well as code that calls your library, and have some clue what the functions do. In C it's common to prefix functions from a given library or module with some common name, i.e. having to do with the type.

StackExchange Code Review Q#6442, answer score: 4