chown doesn't release the FTS handle when exiting, causing a memory leak
fts_close must be called before calling exit()
Details
- Reviewers
imp kevans des anaelle.cazuc_stormshield.eu
before:
$ valgrind --leak-check=full usr.sbin/chown/chown anaellec ~/a ==89707== Memcheck, a memory error detector ==89707== Copyright (C) 2002-2026, and GNU GPL'd, by Julian Seward et al. ==89707== Using Valgrind-3.27.1 and LibVEX; rerun with -h for copyright info ==89707== Command: usr.sbin/chown/chown anaellec /home/anaellec/a ==89707== ==89707== ==89707== HEAP SUMMARY: ==89707== in use at exit: 8,250 bytes in 8 blocks ==89707== total heap usage: 76 allocs, 68 frees, 54,127 bytes allocated ==89707== ==89707== 3,728 (2,448 direct, 1,280 indirect) bytes in 1 blocks are definitely lost in loss record 8 of 8 ==89707== at 0x48580F8: calloc (vg_replace_malloc.c:1620) ==89707== by 0x4913797: fts_open (src/lib/libc/gen/fts.c:233) ==89707== by 0x40023EA: main (usr.sbin/chown/chown.c:162) ==89707== ==89707== LEAK SUMMARY: ==89707== definitely lost: 2,448 bytes in 1 blocks ==89707== indirectly lost: 1,280 bytes in 1 blocks ==89707== possibly lost: 0 bytes in 0 blocks ==89707== still reachable: 4,522 bytes in 6 blocks ==89707== suppressed: 0 bytes in 0 blocks ==89707== Reachable blocks (those to which a pointer was found) are not shown. ==89707== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==89707== ==89707== For lists of detected and suppressed errors, rerun with: -s ==89707== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
after:
$ valgrind --leak-check=full usr.sbin/chown/chown anaellec ~/a ==16283== Memcheck, a memory error detector ==16283== Copyright (C) 2002-2026, and GNU GPL'd, by Julian Seward et al. ==16283== Using Valgrind-3.27.1 and LibVEX; rerun with -h for copyright info ==16283== Command: usr.sbin/chown/chown anaellec /home/anaellec/a ==16283== ==16283== ==16283== HEAP SUMMARY: ==16283== in use at exit: 4,522 bytes in 6 blocks ==16283== total heap usage: 76 allocs, 70 frees, 54,127 bytes allocated ==16283== ==16283== LEAK SUMMARY: ==16283== definitely lost: 0 bytes in 0 blocks ==16283== indirectly lost: 0 bytes in 0 blocks ==16283== possibly lost: 0 bytes in 0 blocks ==16283== still reachable: 4,522 bytes in 6 blocks ==16283== suppressed: 0 bytes in 0 blocks ==16283== Reachable blocks (those to which a pointer was found) are not shown. ==16283== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==16283== ==16283== For lists of detected and suppressed errors, rerun with: -s ==16283== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
I don't object to the change, but the description is incorrect. Calling fts_close(3) before exit(2) is not required, and in fact the patch still leaves numerous paths to termination without an fts_close(3) call, e.g. on the line just above the added call. The only practical benefit is to reduce noise from leak detectors, which may make finding real leaks easier.
Maybe we could leverage atexit to ensure fts_close is called for all the termination paths?
We could, but it's not worth the added complexity. Again, there is absolutely nothing wrong with skipping fts_close() before exit() since exit() already frees all memory and closes all file descriptors, and does so more efficiently than fts_close().
You're right. I didn't see that at first. Let's abandon this patch and get back to more useful work :)