Skip to content

Commit e86afe8

Browse files
committed
feat: add debug memory tracker and harden allocations
Introduce a thread-safe memory tracking system in debug builds and harden dynamic allocations, sanitizing cache and input sizes. Memory Tracking & Cleanup: - Implement thread-safe allocation tracking in debug builds using mutex-protected wrappers for CALLOC, REALLOC, STRDUP, STRNDUP, REALPATH, and FREE. - Track source file, function, and line numbers of all allocations in a singly-linked list. - Register mem_cleanup via atexit in config.c to release remaining tracked memory and headers at exit. Robust Allocations & Failure Hardening: - Harden LinkTable_add, add_arg, and write_memory_callback by checking reallocations for success before incrementing state counters (size, argc, curr_size). - Prevent integer overflows in write_memory_callback size checks. Cache & Link Table Sanitization: - Rename LinkTable.num to LinkTable.size for clarity. - Sanitize the loaded disk link table size in LinkTable_disk_open, deleting the corrupted cache and returning NULL on invalid size. - Simplify segment count verification in Meta_read, checking against dynamic limits and returning EBADMSG on invalid sizes. Option Removal & Bug Fixes: - Remove --max-seg-count CLI option and CONFIG.max_segbc. - Update getopt indices in parse_arg_list to avoid mismatches. - Fix a memory leak in sonic_gen_auth_str by freeing pwd_salt. - Remove outdated staged files note from clang-format hook name.
1 parent 23ec785 commit e86afe8

12 files changed

Lines changed: 504 additions & 151 deletions

File tree

.pre-commit-config.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ repos:
2626
- repo: local
2727
hooks:
2828
- id: clang-format
29-
name: clang-format (formatted files must be manual re-staged)
29+
name: clang-format
3030
entry: clang-format -i
3131
language: system
3232
files: \.(c|h)$

src/cache.c

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ char *CacheSystem_get_cache_dir(void)
4848

4949
const char *xdg_cache_home = getenv("XDG_CACHE_HOME");
5050
if (xdg_cache_home) {
51-
cache_dir = strndup(xdg_cache_home, PATH_MAX);
51+
cache_dir = STRNDUP(xdg_cache_home, PATH_MAX);
5252
} else {
5353
const char *user_home = getenv("HOME");
5454
if (user_home) {
@@ -59,9 +59,10 @@ char *CacheSystem_get_cache_dir(void)
5959
* XDG_CACHE_HOME and HOME already are full paths. Not relying
6060
* on environment PWD since it too may be undefined.
6161
*/
62-
const char *cur_dir = realpath("./", NULL);
62+
char *cur_dir = REALPATH("./", NULL);
6363
if (cur_dir) {
6464
cache_dir = path_append(cur_dir, default_cache_subdir);
65+
FREE(cur_dir);
6566
} else {
6667
lprintf(fatal, "Could not create cache directory\n");
6768
}
@@ -216,8 +217,6 @@ static int Meta_read(Cache *cf)
216217
return EIO;
217218
}
218219

219-
int nmemb = 0;
220-
221220
if (1 != fread(&cf->time, sizeof(long), 1, fp)
222221
|| 1 != fread(&cf->content_length, sizeof(off_t), 1, fp)
223222
|| 1 != fread(&cf->blksz, sizeof(int), 1, fp)
@@ -238,16 +237,36 @@ static int Meta_read(Cache *cf)
238237
lprintf(warning, "Warning: cf->blksz != CONFIG.data_blksz\n");
239238
}
240239

241-
if (cf->segbc > CONFIG.max_segbc) {
242-
lprintf(error, "Error: segbc: %ld\n", cf->segbc);
243-
return EFBIG;
240+
if (cf->content_length <= 0 || cf->blksz <= 0) {
241+
lprintf(error, "Error: invalid metadata sizes\n");
242+
return EBADMSG;
243+
}
244+
245+
if (cf->content_length > INT64_MAX - cf->blksz) {
246+
lprintf(error, "Error: segbc upper bound overflow\n");
247+
return EBADMSG;
248+
}
249+
250+
off_t max_segbc = cf->content_length / cf->blksz;
251+
252+
if ((cf->content_length % cf->blksz) != 0) {
253+
max_segbc += 1;
254+
}
255+
256+
if (max_segbc > INT_MAX) {
257+
max_segbc = INT_MAX;
258+
}
259+
260+
if (cf->segbc <= 0 || cf->segbc > max_segbc) {
261+
lprintf(error, "Error: invalid segbc size: %ld\n", cf->segbc);
262+
return EBADMSG;
244263
}
245264

246265
/*
247266
* Allocate memory for all segments, and read them in
248267
*/
249268
cf->seg = CALLOC(cf->segbc, sizeof(Seg));
250-
nmemb = fread(cf->seg, sizeof(Seg), cf->segbc, fp);
269+
long nmemb = fread(cf->seg, sizeof(Seg), cf->segbc, fp);
251270

252271
/*
253272
* We shouldn't have gone past the end of the file
@@ -256,8 +275,7 @@ static int Meta_read(Cache *cf)
256275
/*
257276
* reached EOF
258277
*/
259-
lprintf(error, "attempted to read past the end of the \
260-
file!\n");
278+
lprintf(error, "attempted to read past the end of the file!\n");
261279
return EBADMSG;
262280
}
263281

@@ -706,7 +724,7 @@ int Cache_create(const char *path)
706724
lprintf(debug, "Creating cache files for %s.\n", fn);
707725

708726
Cache *cf = Cache_alloc();
709-
cf->path = strndup(fn, PATH_MAX);
727+
cf->path = STRNDUP(fn, PATH_MAX);
710728
cf->time = this_link->time;
711729
cf->content_length = this_link->content_length;
712730
cf->blksz = CONFIG.data_blksz;
@@ -814,7 +832,7 @@ Cache *Cache_open(const char *fn)
814832
fn = link->sonic.id;
815833
}
816834

817-
cf->path = strndup(fn, PATH_MAX);
835+
cf->path = STRNDUP(fn, PATH_MAX);
818836

819837
/*
820838
* Associate the cache structure with a link

src/config.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
#include "config.h"
22

33
#include "log.h"
4+
#include "util.h"
45
#include <stddef.h>
6+
#include <stdlib.h>
57

68

79
ConfigStruct CONFIG;
@@ -48,8 +50,6 @@ void Config_init(void)
4850

4951
CONFIG.data_blksz = DEFAULT_DATA_BLKSZ;
5052

51-
CONFIG.max_segbc = DEFAULT_MAX_SEGBC;
52-
5353
/*-------------- Sonic related -------------*/
5454
CONFIG.sonic_username = NULL;
5555

@@ -58,4 +58,5 @@ void Config_init(void)
5858
CONFIG.sonic_id3 = 0;
5959

6060
CONFIG.sonic_insecure = 0;
61+
atexit(mem_cleanup);
6162
}

src/config.h

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,6 @@
3333
*/
3434
#define DEFAULT_DATA_BLKSZ (DEFAULT_DATA_BLKSZ_MB * 1024 * 1024)
3535

36-
/**
37-
* \brief Maximum segment block count
38-
* \details This is set to 128*1024 blocks, which uses 128KB. By default,
39-
* this allows the user to store (128*1024)*(8*1024*1024) = 1TB of data
40-
*/
41-
#define DEFAULT_MAX_SEGBC (128 * 1024)
42-
4336
#define STR(x) #x
4437
#define XSTR(x) STR(x)
4538

src/fuse_local.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ static int fs_readdir(const char *path, void *buf, fuse_fill_dir_t dir_add,
167167
dir_add(buf, ".", NULL, 0, 0);
168168
dir_add(buf, "..", NULL, 0, 0);
169169
/* We skip the head link */
170-
for (int i = 1; i < linktbl->num; i++) {
170+
for (int i = 1; i < linktbl->size; i++) {
171171
Link *link = linktbl->links[i];
172172
if (link->type != LINK_INVALID) {
173173
dir_add(buf, link->linkname, NULL, 0, 0);

src/link.c

Lines changed: 66 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ static void LinkTable_uninitialised_fill(LinkTable *linktbl)
242242
* Start all uninitialized requests once
243243
*/
244244
int total_uninitialized = 0;
245-
for (int i = 0; i < linktbl->num; i++) {
245+
for (int i = 0; i < linktbl->size; i++) {
246246
Link *this_link = linktbl->links[i];
247247
if (this_link->type == LINK_UNINITIALISED_FILE
248248
|| this_link->type == LINK_UNINITIALISED_DIR) {
@@ -260,7 +260,7 @@ static void LinkTable_uninitialised_fill(LinkTable *linktbl)
260260
int j = 0;
261261
do {
262262
u = 0;
263-
for (int i = 0; i < linktbl->num; i++) {
263+
for (int i = 0; i < linktbl->size; i++) {
264264
Link *this_link = linktbl->links[i];
265265
if (this_link->type == LINK_UNINITIALISED_FILE
266266
|| this_link->type == LINK_UNINITIALISED_DIR) {
@@ -284,7 +284,7 @@ static void LinkTable_uninitialised_fill(LinkTable *linktbl)
284284
int n_running = curl_multi_perform_once();
285285

286286
if (n_running == 0) {
287-
for (int i = 0; i < linktbl->num; i++) {
287+
for (int i = 0; i < linktbl->size; i++) {
288288
Link *this_link = linktbl->links[i];
289289
if (this_link->type == LINK_UNINITIALISED_FILE
290290
|| this_link->type == LINK_UNINITIALISED_DIR) {
@@ -367,14 +367,10 @@ LinkTable *LinkSystem_init(const char *url)
367367

368368
void LinkTable_add(LinkTable *linktbl, Link *link)
369369
{
370-
linktbl->num++;
371-
Link **tmp = (Link **)realloc((void *)linktbl->links,
372-
linktbl->num * sizeof(Link *));
373-
if (!tmp) {
374-
lprintf(fatal, "realloc() failure!\n");
375-
}
376-
linktbl->links = tmp;
377-
linktbl->links[linktbl->num - 1] = link;
370+
linktbl->links = (Link **)REALLOC(
371+
(void *)linktbl->links, ((size_t)linktbl->size + 1) * sizeof(Link *));
372+
linktbl->links[linktbl->size] = link;
373+
linktbl->size++;
378374
}
379375

380376
static LinkType linkname_to_LinkType(const char *linkname)
@@ -479,7 +475,7 @@ static void HTML_to_LinkTable(const char *url, GumboNode *node,
479475
if ((type == LINK_UNINITIALISED_DIR)
480476
|| (type == LINK_UNINITIALISED_FILE)) {
481477
int identical_link_found = 0;
482-
for (int i = 0; i < linktbl->num; i++) {
478+
for (int i = 0; i < linktbl->size; i++) {
483479
if (linknames_equal(relative_url,
484480
linktbl->links[i]->linkname)) {
485481
identical_link_found = 1;
@@ -544,7 +540,7 @@ static void LinkTable_fill(LinkTable *linktbl)
544540
{
545541
Link *head_link = linktbl->links[0];
546542
lprintf(debug, "Filling %s\n", head_link->f_url);
547-
for (int i = 1; i < linktbl->num; i++) {
543+
for (int i = 1; i < linktbl->size; i++) {
548544
Link *this_link = linktbl->links[i];
549545
/* Some web sites use characters in their href attributes that really
550546
shouldn't be in their href attributes, most commonly spaces. And
@@ -582,9 +578,13 @@ static void LinkTable_fill(LinkTable *linktbl)
582578
void LinkTable_free(LinkTable *linktbl)
583579
{
584580
if (linktbl) {
585-
for (int i = 0; i < linktbl->num; i++) {
586-
LinkTable_free(linktbl->links[i]->next_table);
587-
FREE(linktbl->links[i]);
581+
for (int i = 0; i < linktbl->size; i++) {
582+
Link *entry = linktbl->links ? linktbl->links[i] : NULL;
583+
if (!entry) {
584+
continue;
585+
}
586+
LinkTable_free(entry->next_table);
587+
FREE(entry);
588588
}
589589
FREE(linktbl->links);
590590
FREE(linktbl);
@@ -599,7 +599,7 @@ void LinkTable_print(LinkTable *linktbl)
599599
lprintf(info, " LinkTable %p for %s\n", (void *)linktbl,
600600
linktbl->links[0]->f_url);
601601
lprintf(info, "--------------------------------------------\n");
602-
for (int i = 0; i < linktbl->num; i++) {
602+
for (int i = 0; i < linktbl->size; i++) {
603603
Link *this_link = linktbl->links[i];
604604
lprintf(info, "%d %c %lu %s %s\n", i, this_link->type,
605605
this_link->content_length, this_link->linkname,
@@ -618,7 +618,7 @@ void LinkTable_print(LinkTable *linktbl)
618618
LinkTable *LinkTable_alloc(const char *url)
619619
{
620620
LinkTable *linktbl = CALLOC(1, sizeof(LinkTable));
621-
linktbl->num = 0;
621+
linktbl->size = 0;
622622
linktbl->index_time = 0;
623623
linktbl->links = NULL;
624624

@@ -629,7 +629,7 @@ LinkTable *LinkTable_alloc(const char *url)
629629
Link *head_link = Link_new("/", LINK_HEAD);
630630
LinkTable_add(linktbl, head_link);
631631
strncpy(head_link->f_url, url, PATH_MAX);
632-
assert(linktbl->num == 1);
632+
assert(linktbl->size == 1);
633633
return linktbl;
634634
}
635635

@@ -743,12 +743,12 @@ int LinkTable_disk_save(LinkTable *linktbl, const char *dirn)
743743
}
744744

745745
lprintf(debug, "linktbl->index_time: %ld\n", (long)linktbl->index_time);
746-
if (fwrite(&linktbl->num, sizeof(int), 1, fp) != 1
746+
if (fwrite(&linktbl->size, sizeof(int), 1, fp) != 1
747747
|| fwrite(&linktbl->index_time, sizeof(time_t), 1, fp) != 1) {
748748
lprintf(error, "Failed to save the header of %s!\n", path);
749749
}
750750
FREE(path);
751-
for (int i = 0; i < linktbl->num; i++) {
751+
for (int i = 0; i < linktbl->size; i++) {
752752
ignore_value(
753753
fwrite(linktbl->links[i]->linkname, sizeof(char), NAME_MAX, fp));
754754
ignore_value(
@@ -788,7 +788,8 @@ LinkTable *LinkTable_disk_open(const char *dirn)
788788
}
789789

790790
LinkTable *linktbl = CALLOC(1, sizeof(LinkTable));
791-
if (fread(&linktbl->num, sizeof(int), 1, fp) != 1
791+
int sz = 0;
792+
if (fread(&sz, sizeof(int), 1, fp) != 1
792793
|| fread(&linktbl->index_time, sizeof(time_t), 1, fp) != 1) {
793794
lprintf(error, "Failed to read the header of %s!\n", path);
794795
fclose(fp);
@@ -797,10 +798,48 @@ LinkTable *LinkTable_disk_open(const char *dirn)
797798
FREE(path);
798799
return NULL;
799800
}
801+
802+
long entry_size = (long)(NAME_MAX + PATH_MAX + sizeof(LinkType)
803+
+ sizeof(size_t) + sizeof(long));
804+
if (fseek(fp, 0, SEEK_END) != 0) {
805+
lprintf(error, "Failed to seek %s!\n", path);
806+
fclose(fp);
807+
LinkTable_free(linktbl);
808+
LinkTable_disk_delete(dirn);
809+
FREE(path);
810+
return NULL;
811+
}
812+
long file_size = ftell(fp);
813+
if (file_size < 0
814+
|| fseek(fp, (long)(sizeof(int) + sizeof(time_t)), SEEK_SET) != 0) {
815+
lprintf(error, "Failed to inspect %s!\n", path);
816+
fclose(fp);
817+
LinkTable_free(linktbl);
818+
LinkTable_disk_delete(dirn);
819+
FREE(path);
820+
return NULL;
821+
}
822+
823+
long max_entries
824+
= (file_size - (long)(sizeof(int) + sizeof(time_t))) / entry_size;
825+
826+
if (sz < 1 || max_entries < sz) {
827+
lprintf(error, "Invalid link table size: %d in %s!\n", sz, path);
828+
fclose(fp);
829+
LinkTable_free(linktbl);
830+
LinkTable_disk_delete(dirn);
831+
FREE(path);
832+
return NULL;
833+
}
834+
835+
linktbl->size = sz;
800836
lprintf(debug, "linktbl->index_time: %ld\n", (long)linktbl->index_time);
801837

802-
linktbl->links = (Link **)CALLOC(linktbl->num, sizeof(Link *));
803-
for (int i = 0; i < linktbl->num; i++) {
838+
linktbl->links
839+
= (Link **)CALLOC( // NOLINT(clang-analyzer-optin.taint.TaintedAlloc)
840+
sz, sizeof(Link *));
841+
842+
for (int i = 0; i < sz; i++) {
804843
linktbl->links[i] = CALLOC(1, sizeof(Link));
805844
if (fread(linktbl->links[i]->linkname, sizeof(char), NAME_MAX, fp)
806845
!= NAME_MAX
@@ -905,7 +944,7 @@ static Link *path_to_Link_recursive(char *path, LinkTable *linktbl)
905944
/*
906945
* We cannot find another '/', we have reached the last level
907946
*/
908-
for (int i = 1; i < linktbl->num; i++) {
947+
for (int i = 1; i < linktbl->size; i++) {
909948
if (!strncmp(path, linktbl->links[i]->linkname, NAME_MAX)) {
910949
/*
911950
* We found our link
@@ -928,7 +967,7 @@ static Link *path_to_Link_recursive(char *path, LinkTable *linktbl)
928967
* move the pointer past the '/'
929968
*/
930969
char *next_path = slash + 1;
931-
for (int i = 1; i < linktbl->num; i++) {
970+
for (int i = 1; i < linktbl->size; i++) {
932971
if (!strncmp(path, linktbl->links[i]->linkname, NAME_MAX)) {
933972
/*
934973
* The next sub-directory exists
@@ -964,7 +1003,7 @@ Link *path_to_Link(const char *path)
9641003
(unsigned long)pthread_self());
9651004

9661005
PTHREAD_MUTEX_LOCK(&link_lock);
967-
char *new_path = strndup(path, PATH_MAX);
1006+
char *new_path = STRNDUP(path, PATH_MAX);
9681007
if (!new_path) {
9691008
lprintf(fatal, "cannot allocate memory\n");
9701009
}

src/link.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ typedef enum {
3333
* \details index 0 contains the Link for the base URL
3434
*/
3535
struct LinkTable {
36-
int num;
36+
int size;
3737
time_t index_time;
3838
Link **links;
3939
};

0 commit comments

Comments
 (0)