@cryptotaxi247 / netdata-1 / commits / 44c0246c6

Array Allocator Memory Leak Fix (#13792)

* aral cleans up previous mmap files; aral uses double linked list macros; aral got more internal checks * fixed aral memory leak * updated macro

Costa Tsaousis committed Oct 10, 2022 at 14:20 UTC 44c0246c6ba4ab0a3bd63342687166dbfdd1b626

4 files changed +134 -97

database/engine/pagecache.c

+2 -2

@@ -4,8 +4,8 @@

4 #include "rrdengine.h"

6 ARAL page_descr_aral = {

7 - .element_size = sizeof(struct rrdeng_page_descr),

8 - .elements = 20000,

7 + .requested_element_size = sizeof(struct rrdeng_page_descr),

8 + .initial_elements = 20000,

9 .filename = "page_descriptors",

10 .cache_dir = &netdata_configured_cache_dir,

11 .use_mmap = false,

libnetdata/arrayalloc/arrayalloc.c

+126 -90

@@ -32,6 +32,33 @@ static inline size_t natural_alignment(size_t size, size_t alignment) {

32 return size;

33 }

35 +static void arrayalloc_delete_leftover_files(const char *path, const char *required_prefix) {

36 + DIR *dir = opendir(path);

37 + if(!dir) return;

38 +

39 + char fullpath[FILENAME_MAX + 1];

40 + size_t len = strlen(required_prefix);

41 +

42 + struct dirent *de = NULL;

43 + while((de = readdir(dir))) {

44 + if(de->d_type == DT_DIR)

45 + continue;

46 +

47 + if(strncmp(de->d_name, required_prefix, len) != 0)

48 + continue;

49 +

50 + snprintfz(fullpath, FILENAME_MAX, "%s/%s", path, de->d_name);

51 + info("ARRAYALLOC: removing left-over file '%s'", fullpath);

52 + if(unlikely(unlink(fullpath) == -1))

53 + error("Cannot delete file '%s'", fullpath);

54 + }

55 +

56 + closedir(dir);

57 +}

58 +

59 +// ----------------------------------------------------------------------------

60 +// arrayalloc_init()

61 +

62 static void arrayalloc_init(ARAL *ar) {

63 static netdata_mutex_t mutex = NETDATA_MUTEX_INITIALIZER;

64 netdata_mutex_lock(&mutex);

@@ -47,7 +74,7 @@ static void arrayalloc_init(ARAL *ar) {

75 // we need to add a page pointer after the element

76 // so, first align the element size to the pointer size

50 - ar->internal.element_size = natural_alignment(ar->element_size, sizeof(uintptr_t));

77 + ar->internal.element_size = natural_alignment(ar->requested_element_size, sizeof(uintptr_t));

79 // then add the size of a pointer to it

80 ar->internal.element_size += sizeof(uintptr_t);

@@ -59,18 +86,18 @@ static void arrayalloc_init(ARAL *ar) {

86 // and finally align it to the natural alignment

87 ar->internal.element_size = natural_alignment(ar->internal.element_size, ARAL_NATURAL_ALIGNMENT);

62 - // this is where we should write the pointer

89 + // we write the page pointer just after each element

90 ar->internal.page_ptr_offset = ar->internal.element_size - sizeof(uintptr_t);

65 - if(ar->element_size + sizeof(uintptr_t) > ar->internal.element_size)

92 + if(ar->requested_element_size + sizeof(uintptr_t) > ar->internal.element_size)

93 fatal("ARRAYALLOC: failed to calculate properly page_ptr_offset: element size %zu, sizeof(uintptr_t) %zu, natural alignment %zu, final element size %zu, page_ptr_offset %zu",

67 - ar->element_size, sizeof(uintptr_t), ARAL_NATURAL_ALIGNMENT, ar->internal.element_size, ar->internal.page_ptr_offset);

94 + ar->requested_element_size, sizeof(uintptr_t), ARAL_NATURAL_ALIGNMENT, ar->internal.element_size, ar->internal.page_ptr_offset);

96 //info("ARRAYALLOC: element size %zu, sizeof(uintptr_t) %zu, natural alignment %zu, final element size %zu, page_ptr_offset %zu",

97 // ar->element_size, sizeof(uintptr_t), ARAL_NATURAL_ALIGNMENT, ar->internal.element_size, ar->internal.page_ptr_offset);

72 - if (ar->elements < 10)

73 - ar->elements = 10;

99 + if (ar->initial_elements < 10)

100 + ar->initial_elements = 10;

101

102 ar->internal.mmap = (ar->use_mmap && ar->cache_dir && *ar->cache_dir) ? true : false;

103 ar->internal.max_alloc_size = ar->internal.mmap ? ARAL_MAX_PAGE_SIZE_MMAP : ARAL_MAX_PAGE_SIZE_MALLOC;

@@ -81,17 +108,20 @@ static void arrayalloc_init(ARAL *ar) {

108 if(ar->internal.max_alloc_size % ar->internal.element_size)

109 ar->internal.max_alloc_size -= ar->internal.max_alloc_size % ar->internal.element_size;

110

84 - ar->internal.first_page = NULL;

85 - ar->internal.last_page = NULL;

111 + ar->internal.pages = NULL;

112 ar->internal.allocation_multiplier = 1;

113 ar->internal.file_number = 0;

114

115 if(ar->internal.mmap) {

90 - char filename[FILENAME_MAX + 1];

91 - snprintfz(filename, FILENAME_MAX, "%s/array_alloc.mmap", *ar->cache_dir);

92 - int r = mkdir(filename, 0775);

116 + char directory_name[FILENAME_MAX + 1];

117 + snprintfz(directory_name, FILENAME_MAX, "%s/array_alloc.mmap", *ar->cache_dir);

118 + int r = mkdir(directory_name, 0775);

119 if (r != 0 && errno != EEXIST)

94 - fatal("Cannot create directory '%s'", filename);

120 + fatal("Cannot create directory '%s'", directory_name);

121 +

122 + char filename[FILENAME_MAX + 1];

123 + snprintfz(filename, FILENAME_MAX, "%s.", ar->filename);

124 + arrayalloc_delete_leftover_files(directory_name, filename);

125 }

126

127 ar->internal.initialized = true;

@@ -100,8 +130,11 @@ static void arrayalloc_init(ARAL *ar) {

130 netdata_mutex_unlock(&mutex);

131 }

132

133 +// ----------------------------------------------------------------------------

134 +// check a free slot

135 +

136 #ifdef NETDATA_INTERNAL_CHECKS

104 -static inline void arrayalloc_free_checks(ARAL *ar, ARAL_FREE *fr) {

137 +static inline void arrayalloc_free_validate_internal_check(ARAL *ar, ARAL_FREE *fr) {

138 if(fr->size < ar->internal.element_size)

139 fatal("ARRAYALLOC: free item of size %zu, less than the expected element size %zu", fr->size, ar->internal.element_size);

140

@@ -109,58 +142,47 @@ static inline void arrayalloc_free_checks(ARAL *ar, ARAL_FREE *fr) {

142 fatal("ARRAYALLOC: free item of size %zu is not multiple to element size %zu", fr->size, ar->internal.element_size);

143 }

144 #else

112 -#define arrayalloc_free_checks(ar, fr) debug_dummy()

145 +#define arrayalloc_free_validate_internal_check(ar, fr) debug_dummy()

146 #endif

147

115 -static inline void unlink_page(ARAL *ar, ARAL_PAGE *page) {

116 - if(unlikely(!page)) return;

117 -

118 - if(page->next)

119 - page->next->prev = page->prev;

120 -

121 - if(page->prev)

122 - page->prev->next = page->next;

148 +// ----------------------------------------------------------------------------

149 +// find the page a pointer belongs to

150

124 - if(page == ar->internal.first_page)

125 - ar->internal.first_page = page->next;

126 -

127 - if(page == ar->internal.last_page)

128 - ar->internal.last_page = page->prev;

129 -}

130 -

131 -static inline void link_page_first(ARAL *ar, ARAL_PAGE *page) {

132 - page->prev = NULL;

133 - page->next = ar->internal.first_page;

134 - if(page->next) page->next->prev = page;

151 +#ifdef NETDATA_INTERNAL_CHECKS

152 +static inline ARAL_PAGE *find_page_with_allocation_internal_check(ARAL *ar, void *ptr) {

153 + uintptr_t seeking = (uintptr_t)ptr;

154 + ARAL_PAGE *page;

155

136 - ar->internal.first_page = page;

156 + for(page = ar->internal.pages; page ; page = page->next) {

157 + if(unlikely(seeking >= (uintptr_t)page->data && seeking < (uintptr_t)page->data + page->size))

158 + break;

159 + }

160

138 - if(!ar->internal.last_page)

139 - ar->internal.last_page = page;

161 + return page;

162 }

163 +#endif

164

142 -static inline void link_page_last(ARAL *ar, ARAL_PAGE *page) {

143 - page->next = NULL;

144 - page->prev = ar->internal.last_page;

145 - if(page->prev) page->prev->next = page;

146 -

147 - ar->internal.last_page = page;

165 +// ----------------------------------------------------------------------------

166 +// find a page with a free slot (there shouldn't be any)

167

149 - if(!ar->internal.first_page)

150 - ar->internal.first_page = page;

151 -}

152 -

153 -static inline ARAL_PAGE *find_page_with_allocation(ARAL *ar, void *ptr) {

154 - uintptr_t seeking = (uintptr_t)ptr;

168 +#ifdef NETDATA_INTERNAL_CHECKS

169 +static inline ARAL_PAGE *find_page_with_free_slots_internal_check(ARAL *ar) {

170 ARAL_PAGE *page;

171

157 - for(page = ar->internal.first_page; page ; page = page->next) {

158 - if(unlikely(seeking >= (uintptr_t)page->data && seeking < (uintptr_t)page->data + page->size))

172 + for(page = ar->internal.pages; page ; page = page->next) {

173 + if(page->free_list)

174 break;

175 +

176 + internal_fatal(page->size - page->used_elements * ar->internal.element_size >= ar->internal.element_size,

177 + "ARRAYALLOC: a page is marked full, but it is not!");

178 +

179 + internal_fatal(page->size < page->used_elements * ar->internal.element_size,

180 + "ARRAYALLOC: a page has been overflown!");

181 }

182

183 return page;

184 }

185 +#endif

186

187 #ifdef NETDATA_TRACE_ALLOCATIONS

188 static void arrayalloc_add_page(ARAL *ar, const char *file, const char *function, size_t line) {

@@ -171,7 +193,7 @@ static void arrayalloc_add_page(ARAL *ar) {

193 arrayalloc_init(ar);

194

195 ARAL_PAGE *page = callocz(1, sizeof(ARAL_PAGE));

174 - page->size = ar->elements * ar->internal.element_size * ar->internal.allocation_multiplier;

196 + page->size = ar->initial_elements * ar->internal.element_size * ar->internal.allocation_multiplier;

197 if(page->size > ar->internal.max_alloc_size)

198 page->size = ar->internal.max_alloc_size;

199 else

@@ -202,9 +224,9 @@ static void arrayalloc_add_page(ARAL *ar) {

224 page->free_list = fr;

225

226 // link the new page at the front of the list of pages

205 - link_page_first(ar, page);

227 + DOUBLE_LINKED_LIST_PREPEND_UNSAFE(ar->internal.pages, page, prev, next);

228

207 - arrayalloc_free_checks(ar, fr);

229 + arrayalloc_free_validate_internal_check(ar, fr);

230 }

231

232 static void arrayalloc_lock(ARAL *ar) {

@@ -217,12 +239,13 @@ static void arrayalloc_unlock(ARAL *ar) {

239 netdata_mutex_unlock(&ar->internal.mutex);

240 }

241

220 -ARAL *arrayalloc_create(size_t element_size, size_t elements, const char *filename, char **cache_dir) {

242 +ARAL *arrayalloc_create(size_t element_size, size_t elements, const char *filename, char **cache_dir, bool mmap) {

243 ARAL *ar = callocz(1, sizeof(ARAL));

222 - ar->element_size = element_size;

223 - ar->elements = elements;

244 + ar->requested_element_size = element_size;

245 + ar->initial_elements = elements;

246 ar->filename = filename;

247 ar->cache_dir = cache_dir;

248 + ar->use_mmap = mmap;

249 return ar;

250 }

251

@@ -236,7 +259,10 @@ void *arrayalloc_mallocz(ARAL *ar) {

259

260 arrayalloc_lock(ar);

261

239 - if(unlikely(!ar->internal.first_page || !ar->internal.first_page->free_list)) {

262 + if(unlikely(!ar->internal.pages || !ar->internal.pages->free_list)) {

263 + internal_fatal(find_page_with_free_slots_internal_check(ar) != NULL,

264 + "ARRAYALLOC: first page does not have any free slots, but there is another that has!");

265 +

266 #ifdef NETDATA_TRACE_ALLOCATIONS

267 arrayalloc_add_page(ar, file, function, line);

268 #else

@@ -244,44 +270,53 @@ void *arrayalloc_mallocz(ARAL *ar) {

270 #endif

271 }

272

247 - ARAL_PAGE *page = ar->internal.first_page;

248 - ARAL_FREE *fr = page->free_list;

273 + ARAL_PAGE *page = ar->internal.pages;

274 + ARAL_FREE *found_fr = page->free_list;

275 +

276 + internal_fatal(!found_fr,

277 + "ARRAYALLOC: free item to use, cannot be NULL.");

278

250 - if(unlikely(!fr))

251 - fatal("ARRAYALLOC: free item cannot be NULL.");

279 + internal_fatal(found_fr->size < ar->internal.element_size,

280 + "ARRAYALLOC: free item size %zu, cannot be smaller than %zu",

281 + found_fr->size, ar->internal.element_size);

282

253 - if(unlikely(fr->size < ar->internal.element_size))

254 - fatal("ARRAYALLOC: free item size %zu is smaller than %zu", fr->size, ar->internal.element_size);

283 + if(unlikely(found_fr->size - ar->internal.element_size < ar->internal.element_size)) {

284 + // we can use the entire free space entry

285

256 - if(fr->size - ar->internal.element_size <= ar->internal.element_size) {

257 - // we are done with this page

258 - page->free_list = NULL;

286 + page->free_list = found_fr->next;

287

260 - if(page != ar->internal.last_page) {

261 - unlink_page(ar, page);

262 - link_page_last(ar, page);

288 + if(unlikely(!page->free_list)) {

289 + // we are done with this page

290 + // move the full page last

291 + // so that pages with free items remain first in the list

292 + DOUBLE_LINKED_LIST_REMOVE_UNSAFE(ar->internal.pages, page, prev, next);

293 + DOUBLE_LINKED_LIST_APPEND_UNSAFE(ar->internal.pages, page, prev, next);

294 }

295 }

296 else {

266 - uint8_t *data = (uint8_t *)fr;

267 - ARAL_FREE *fr2 = (ARAL_FREE *)&data[ar->internal.element_size];

268 - fr2->page = fr->page;

269 - fr2->size = fr->size - ar->internal.element_size;

270 - fr2->next = fr->next;

271 - page->free_list = fr2;

272 -

273 - arrayalloc_free_checks(ar, fr2);

297 + // we can split the free space entry

298 +

299 + uint8_t *data = (uint8_t *)found_fr;

300 + ARAL_FREE *fr = (ARAL_FREE *)&data[ar->internal.element_size];

301 + fr->page = page;

302 + fr->size = found_fr->size - ar->internal.element_size;

303 +

304 + // link the free slot first in the page

305 + fr->next = found_fr->next;

306 + page->free_list = fr;

307 +

308 + arrayalloc_free_validate_internal_check(ar, fr);

309 }

310

276 - fr->page->used_elements++;

311 + page->used_elements++;

312

313 // put the page pointer after the element

279 - uint8_t *data = (uint8_t *)fr;

314 + uint8_t *data = (uint8_t *)found_fr;

315 ARAL_PAGE **page_ptr = (ARAL_PAGE **)&data[ar->internal.page_ptr_offset];

316 *page_ptr = page;

317

318 arrayalloc_unlock(ar);

284 - return (void *)fr;

319 + return (void *)found_fr;

320 }

321

322 #ifdef NETDATA_TRACE_ALLOCATIONS

@@ -289,7 +324,7 @@ void arrayalloc_freez_int(ARAL *ar, void *ptr, const char *file, const char *fun

324 #else

325 void arrayalloc_freez(ARAL *ar, void *ptr) {

326 #endif

292 - if(!ptr) return;

327 + if(unlikely(!ptr)) return;

328 arrayalloc_lock(ar);

329

330 // get the page pointer

@@ -310,7 +345,7 @@ void arrayalloc_freez(ARAL *ar, void *ptr) {

345 #ifdef NETDATA_INTERNAL_CHECKS

346 {

347 // find the page ptr belongs

313 - ARAL_PAGE *page2 = find_page_with_allocation(ar, ptr);

348 + ARAL_PAGE *page2 = find_page_with_allocation_internal_check(ar, ptr);

349

350 if(unlikely(page != page2))

351 fatal("ARRAYALLOC: page pointers do not match!");

@@ -337,7 +372,7 @@ void arrayalloc_freez(ARAL *ar, void *ptr) {

372

373 // if the page is empty, release it

374 if(!page->used_elements) {

340 - unlink_page(ar, page);

375 + DOUBLE_LINKED_LIST_REMOVE_UNSAFE(ar->internal.pages, page, prev, next);

376

377 // free it

378 if(ar->internal.mmap) {

@@ -356,9 +391,11 @@ void arrayalloc_freez(ARAL *ar, void *ptr) {

391

392 freez(page);

393 }

359 - else if(page != ar->internal.first_page) {

360 - unlink_page(ar, page);

361 - link_page_first(ar, page);

394 + else if(page != ar->internal.pages) {

395 + // move the page with free item first

396 + // so that the next allocation will use this page

397 + DOUBLE_LINKED_LIST_REMOVE_UNSAFE(ar->internal.pages, page, prev, next);

398 + DOUBLE_LINKED_LIST_PREPEND_UNSAFE(ar->internal.pages, page, prev, next);

399 }

400

401 arrayalloc_unlock(ar);

@@ -366,8 +403,7 @@ void arrayalloc_freez(ARAL *ar, void *ptr) {

403

404 int aral_unittest(size_t elements) {

405 char *cache_dir = "/tmp/";

369 - ARAL *ar = arrayalloc_create(20, 10, "test-aral", &cache_dir);

370 - ar->use_mmap = false;

406 + ARAL *ar = arrayalloc_create(20, 10, "test-aral", &cache_dir, false);

407

408 void *pointers[elements];

409

@@ -399,7 +435,7 @@ int aral_unittest(size_t elements) {

435 arrayalloc_freez(ar, pointers[i]);

436 }

437

402 - if(ar->internal.first_page) {

438 + if(ar->internal.pages) {

439 fprintf(stderr, "ARAL leftovers detected (1)");

440 return 1;

441 }

@@ -442,7 +478,7 @@ int aral_unittest(size_t elements) {

478

479 arrayalloc_freez(ar, pointers[allocated - 1]);

480

445 - if(ar->internal.first_page) {

481 + if(ar->internal.pages) {

482 fprintf(stderr, "ARAL leftovers detected (2)");

483 return 1;

484 }

libnetdata/arrayalloc/arrayalloc.h

+4 -5

@@ -5,8 +5,8 @@

5 #include "../libnetdata.h"

7 typedef struct arrayalloc {

8 - size_t element_size;

9 - size_t elements;

8 + size_t requested_element_size;

9 + size_t initial_elements;

10 const char *filename;

11 char **cache_dir;

12 bool use_mmap;

@@ -23,12 +23,11 @@ typedef struct arrayalloc {

23 size_t allocation_multiplier;

24 size_t max_alloc_size;

25 netdata_mutex_t mutex;

26 - struct arrayalloc_page *first_page;

27 - struct arrayalloc_page *last_page;

26 + struct arrayalloc_page *pages;

27 } internal;

28 } ARAL;

31 -ARAL *arrayalloc_create(size_t element_size, size_t elements, const char *filename, char **cache_dir);

30 +ARAL *arrayalloc_create(size_t element_size, size_t elements, const char *filename, char **cache_dir, bool mmap);

31 int aral_unittest(size_t elements);

33 #ifdef NETDATA_TRACE_ALLOCATIONS

libnetdata/log/log.h

@@ -87,9 +87,11 @@ void error_log_limit_unlimited(void);

87 #ifdef NETDATA_INTERNAL_CHECKS

88 #define debug(type, args...) do { if(unlikely(debug_flags & type)) debug_int(__FILE__, __FUNCTION__, __LINE__, ##args); } while(0)

89 #define internal_error(condition, args...) do { if(unlikely(condition)) error_int("IERR", __FILE__, __FUNCTION__, __LINE__, ##args); } while(0)

90 +#define internal_fatal(condition, args...) do { if(unlikely(condition)) fatal_int(__FILE__, __FUNCTION__, __LINE__, ##args); } while(0)

91 #else

92 #define debug(type, args...) debug_dummy()

93 #define internal_error(args...) debug_dummy()

94 +#define internal_fatal(args...) debug_dummy()

95 #endif

97 #define info(args...) info_int(__FILE__, __FUNCTION__, __LINE__, ##args)