memleak in mnt_table_parse_swaps()

[keszybz]

Hi,

I was writing a unittest for systemd, and found what seems to be a small memleak in mnt_table_parse_swaps(). I'm parsing the following dummy file:

Filename				Type		Size	Used	Priority
/dev/dm-2                               partition	8151036	2283436	-2
/some/swapfile                          file            111     111     0
/some/swapfile2 (deleted)               file            111     111     0

and get the following report from valgrind:

    ==26912== 16 bytes in 1 blocks are definitely lost in loss record 1 of 1
    ==26912==    at 0x4C31C15: realloc (vg_replace_malloc.c:785)
    ==26912==    by 0x55C5D8C: _IO_vfscanf (in /usr/lib64/libc-2.26.so)
    ==26912==    by 0x55D8AEC: vsscanf (in /usr/lib64/libc-2.26.so)
    ==26912==    by 0x55D25C3: sscanf (in /usr/lib64/libc-2.26.so)
    ==26912==    by 0x53236D0: mnt_table_parse_stream (in /usr/lib64/libmount.so.1.1.0)
    ==26912==    by 0x53249B6: mnt_table_parse_file (in /usr/lib64/libmount.so.1.1.0)
    ==26912==    by 0x10D157: swap_list_get (umount.c:194)
    ==26912==    by 0x10B06E: test_swap_list (test-umount.c:34)
    ==26912==    by 0x10B24B: main (test-umount.c:56)

The memleaks comes from the line with " (deleted)". If I duplicate that line, valgrind reports a lost block for each copy.

I'll push my PR to systemd shortly, so a reference to it should pop up here in case you want to see the code.

[karelzak]

@keszybz I have doubts the example is a real copy of the /proc/swaps file. The kernel escapes spaces in paths, so the expected is:

/some/swapfile2\040(deleted)               file            111     111     0

The libmount will interpret a line without \040 as parse error...

Anyway, there was bug in the parser; on parse-error it does not deallocate the path. So, thanks for report.

[keszybz]

I have doubts the example is a real copy of the /proc/swaps file

It's not... It came from a crafted test file.

[evverx]

@keszybz, I'm not sure what the idea behind that space is, but it seems it doesn't matter what is placed there. If so, would it make sense to change the space to \040, so that the test won't fail under valgrind and asan?

[karelzak]

More details, kernel uses d_path() function to convert dentry to path string. The function appends " (deleted)" to the path for unlinked files. The string is later escaped (space is converted to \040) by seq_file_path() when mm/swapfile.c generates /proc/swaps. This is reason why the file in the test seem incorrect for me. It seems like output from old swapon rather than cp from /proc.

Yes, change the space to \040 will fix the problem for old libmount versions too. The next version will be robust enough to avoid the leak.