asprintf(3) error handling, mostly #11993

nabijaczleweli · 2021-05-03T10:36:48Z

Motivation and Context

If asprintf(&p) == -1, the value of p is undefined. And we all know how much fun freeing random pointers is.

Description

The first patch cleans up libzfs/zpool_load_compat() by:

initalising l_features with an initialiser list instead of a for loop
passing file open flags to file open flags (and adding O_CLOEXEC to the now flags flags)
correctly detecting a failed map
prefaulting the whole file (supported on both Linux (MAP_POPULATE) and FreeBSD (MAP_PREFAULT_READ)), since we're writing to every page anyway

The fourth patch removes a deliberate leak(!?) and useless short path from zutil_device_path/linux/zfs_get_enclosure_sysfs_path()

The rest fix asprintf(3) usage. If you wanna have a go, too: git grep asprintf | grep -vE '(zfs_|kmem_|kv|kmem_v)asprintf'.
I didn't touch extraneous ret == -1 || p == NULL error checks.

Also, zfs_asprintf() and zfs_alloc() say "which will die if the allocation fails", which is strictly false. no_memory() in the same file says "Display an out of memory error message and abort the current program.", which is also wrong, on both counts.

How Has This Been Tested?

Looked at it. Tested some ex situ, but this hardly requires a week-long test regime.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes. – none apply
I have run the ZFS Test Suite with this change applied. – CI take my hand
All commit messages are properly formatted and contain Signed-off-by.

As a bonus, this also passes the open flags into the open flags instead of the mode (it worked by accident because O_RDONLY is 0), correctly detects a failed map, and prefaults the entire file since we're always writing to every page Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

…ev path Also always free tmp2 at the end Before: nabijaczleweli@tarta:~/uwu$ valgrind --leak-check=full ./blergh ==8947== Memcheck, a memory error detector ==8947== Using Valgrind-3.14.0 and LibVEX ==8947== Command: ./blergh ==8947== (null) ==8947== ==8947== HEAP SUMMARY: ==8947== in use at exit: 23 bytes in 1 blocks ==8947== total heap usage: 3 allocs, 2 frees, 1,147 bytes allocated ==8947== ==8947== 23 bytes in 1 blocks are definitely lost in loss record 1 of 1 ==8947== at 0x483577F: malloc (vg_replace_malloc.c:299) ==8947== by 0x48D74B7: vasprintf (vasprintf.c:73) ==8947== by 0x48B7833: asprintf (asprintf.c:35) ==8947== by 0x401258: zfs_get_enclosure_sysfs_path (zutil_device_path_os.c:191) ==8947== by 0x401482: main (blergh.c:107) ==8947== ==8947== LEAK SUMMARY: ==8947== definitely lost: 23 bytes in 1 blocks ==8947== indirectly lost: 0 bytes in 0 blocks ==8947== possibly lost: 0 bytes in 0 blocks ==8947== still reachable: 0 bytes in 0 blocks ==8947== suppressed: 0 bytes in 0 blocks ==8947== ==8947== For counts of detected and suppressed errors, rerun with: -v ==8947== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) nabijaczleweli@tarta:~/uwu$ sed -n 191p zutil_device_path_os.c tmpsize = asprintf(&tmp1, "/sys/block/%s/device", dev_name); After: nabijaczleweli@tarta:~/uwu$ valgrind --leak-check=full ./blergh ==9512== Memcheck, a memory error detector ==9512== Using Valgrind-3.14.0 and LibVEX ==9512== Command: ./blergh ==9512== (null) ==9512== ==9512== HEAP SUMMARY: ==9512== in use at exit: 0 bytes in 0 blocks ==9512== total heap usage: 3 allocs, 3 frees, 1,147 bytes allocated ==9512== ==9512== All heap blocks were freed -- no leaks are possible ==9512== ==9512== For counts of detected and suppressed errors, rerun with: -v ==9512== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

…ndefined pointer Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

behlendorf

Thanks for the cleanup. This LGTM, I've resubmitted the FreeBSD builds when they come back clean this is good to go.

behlendorf · 2021-05-08T16:37:03Z

lib/libzfs/libzfs_util.c

 		(void) no_memory(hdl);
+		ret = NULL;


Also, zfs_asprintf() and zfs_alloc() say "which will die if the allocation fails", which is strictly false. no_memory() in the same file says "Display an out of memory error message and abort the current program.", which is also wrong, on both counts.

If you trace the call path you'll see this is in fact true when hdl->libzfs_printerr is set. But clearly we can't count on that so I agree setting this to NULL is the right thing to do here.

no_memory() zfs_error(, EZFS_NOMEM, ) zfs_error_fmt() zfs_verror() exit()

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11993

Also always free tmp2 at the end Before: nabijaczleweli@tarta:~/uwu$ valgrind --leak-check=full ./blergh ==8947== Memcheck, a memory error detector ==8947== Using Valgrind-3.14.0 and LibVEX ==8947== Command: ./blergh ==8947== (null) ==8947== ==8947== HEAP SUMMARY: ==8947== in use at exit: 23 bytes in 1 blocks ==8947== total heap usage: 3 allocs, 2 frees, 1,147 bytes allocated ==8947== ==8947== 23 bytes in 1 blocks are definitely lost in loss record 1 of 1 ==8947== at 0x483577F: malloc (vg_replace_malloc.c:299) ==8947== by 0x48D74B7: vasprintf (vasprintf.c:73) ==8947== by 0x48B7833: asprintf (asprintf.c:35) ==8947== by 0x401258: zfs_get_enclosure_sysfs_path (zutil_device_path_os.c:191) ==8947== by 0x401482: main (blergh.c:107) ==8947== ==8947== LEAK SUMMARY: ==8947== definitely lost: 23 bytes in 1 blocks ==8947== indirectly lost: 0 bytes in 0 blocks ==8947== possibly lost: 0 bytes in 0 blocks ==8947== still reachable: 0 bytes in 0 blocks ==8947== suppressed: 0 bytes in 0 blocks ==8947== ==8947== For counts of detected and suppressed errors, rerun with: -v ==8947== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) nabijaczleweli@tarta:~/uwu$ sed -n 191p zutil_device_path_os.c tmpsize = asprintf(&tmp1, "/sys/block/%s/device", dev_name); After: nabijaczleweli@tarta:~/uwu$ valgrind --leak-check=full ./blergh ==9512== Memcheck, a memory error detector ==9512== Using Valgrind-3.14.0 and LibVEX ==9512== Command: ./blergh ==9512== (null) ==9512== ==9512== HEAP SUMMARY: ==9512== in use at exit: 0 bytes in 0 blocks ==9512== total heap usage: 3 allocs, 3 frees, 1,147 bytes allocated ==9512== ==9512== All heap blocks were freed -- no leaks are possible ==9512== ==9512== For counts of detected and suppressed errors, rerun with: -v ==9512== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11993

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11993

As a bonus, this also passes the open flags into the open flags instead of the mode (it worked by accident because O_RDONLY is 0), correctly detects a failed map, and prefaults the entire file since we're always writing to every page Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11993

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11993

Also always free tmp2 at the end Before: nabijaczleweli@tarta:~/uwu$ valgrind --leak-check=full ./blergh ==8947== Memcheck, a memory error detector ==8947== Using Valgrind-3.14.0 and LibVEX ==8947== Command: ./blergh ==8947== (null) ==8947== ==8947== HEAP SUMMARY: ==8947== in use at exit: 23 bytes in 1 blocks ==8947== total heap usage: 3 allocs, 2 frees, 1,147 bytes allocated ==8947== ==8947== 23 bytes in 1 blocks are definitely lost in loss record 1 of 1 ==8947== at 0x483577F: malloc (vg_replace_malloc.c:299) ==8947== by 0x48D74B7: vasprintf (vasprintf.c:73) ==8947== by 0x48B7833: asprintf (asprintf.c:35) ==8947== by 0x401258: zfs_get_enclosure_sysfs_path (zutil_device_path_os.c:191) ==8947== by 0x401482: main (blergh.c:107) ==8947== ==8947== LEAK SUMMARY: ==8947== definitely lost: 23 bytes in 1 blocks ==8947== indirectly lost: 0 bytes in 0 blocks ==8947== possibly lost: 0 bytes in 0 blocks ==8947== still reachable: 0 bytes in 0 blocks ==8947== suppressed: 0 bytes in 0 blocks ==8947== ==8947== For counts of detected and suppressed errors, rerun with: -v ==8947== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) nabijaczleweli@tarta:~/uwu$ sed -n 191p zutil_device_path_os.c tmpsize = asprintf(&tmp1, "/sys/block/%s/device", dev_name); After: nabijaczleweli@tarta:~/uwu$ valgrind --leak-check=full ./blergh ==9512== Memcheck, a memory error detector ==9512== Using Valgrind-3.14.0 and LibVEX ==9512== Command: ./blergh ==9512== (null) ==9512== ==9512== HEAP SUMMARY: ==9512== in use at exit: 0 bytes in 0 blocks ==9512== total heap usage: 3 allocs, 3 frees, 1,147 bytes allocated ==9512== ==9512== All heap blocks were freed -- no leaks are possible ==9512== ==9512== For counts of detected and suppressed errors, rerun with: -v ==9512== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11993

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11993

colmbuckley · 2021-05-28T21:50:31Z

I'm slightly confused by the initialization of l_features using {B_FALSE}; if I'm reading the C99 spec correctly that initializes the first element to B_FALSE and the remaining elements to 0. Now obviously B_FALSE is 0, but is that something it's safe to count on forever?

behlendorf · 2021-05-28T22:29:54Z

Practically speaking it's something we're very unlikely to change, but you never know it might end up being a problem on other platforms. I'd be fine with explicitly initializing everything to B_FALSE as was done before if we want to be extra safe. @colmbuckley sorry for not cc'ing you on this PR when it was originally opened.

colmbuckley · 2021-05-29T11:55:16Z

@behlendorf No need to apologize; I'm delighted that others are reviewing the code and catching problems, especially before a stable release; and the openat() bug was a real thing - thanks a million to @nabijaczleweli for catching that. I also learned something about the prefaulting mechanism of mmap() so double win.

I'll open a tiny PR to look at the initialization issue and a couple of other minor style issues I've noticed.