Don't unescape paths from libmount by keszybz · Pull Request #12252 · systemd/systemd

keszybz · 2019-04-08T15:00:02Z

The first three commits from #12218.

Use a trivial header file to share mnt_free_tablep and mnt_free_iterp. It would be nicer put this in mount-util.h, but libmount.h is not in the default include path, and the build system would have to be adjusted to pass pkg-config include path in various places, and it's just not worth the trouble. A separate header file works nicely.

keszybz · 2019-04-08T21:14:33Z

Updated. The test is updated to ignore errors more consistently. No other changes.

With libmount-2.33.1-3.fc30.x86_64 I get: /* test_libmount_unescaping_one escaped space + utf8 */ from '729 38 0:59 / /tmp/\342\200\236zupa\\040z\304\231bowa\342\200\235 rw,relatime shared:395 - tmpfs die\\040Br\303\274he rw,seclabel' source: 'die Brühe' source: 'die Br\303\274he' source: 'die Brühe' expected: 'die Brühe' target: '/tmp/„zupa zębowa”' target: '/tmp/\342\200\236zupa z\304\231bowa\342\200\235' target: '/tmp/„zupa zębowa”' expected: '/tmp/„zupa zębowa”' /* test_libmount_unescaping_one escaped newline */ from '729 38 0:59 / /tmp/x\\012y rw,relatime shared:395 - tmpfs newline rw,seclabel' source: 'newline' source: 'newline' source: 'newline' expected: 'newline' target: '/tmp/x y' target: '/tmp/x\ny' target: '/tmp/x y' expected: '/tmp/x y' /* test_libmount_unescaping_one empty source */ from '760 38 0:60 / /tmp/emptysource rw,relatime shared:410 - tmpfs rw,seclabel' source: '' source: '' source: '' expected: '' target: '/tmp/emptysource' target: '/tmp/emptysource' target: '/tmp/emptysource' expected: '/tmp/emptysource' /* test_libmount_unescaping_one foo\rbar */ from '790 38 0:61 / /tmp/foo\rbar rw,relatime shared:425 - tmpfs tmpfs rw,seclabel' source: 'tmpfs' source: 'tmpfs' source: 'tmpfs' expected: 'tmpfs' target: '/tmp/foo' target: '/tmp/foo' target: '/tmp/foo' expected: 'n/a' With util-linux/util-linux#780 fixed, we get /* test_libmount_unescaping_one foo\rbar */ from '790 38 0:61 / /tmp/foo\rbar rw,relatime shared:425 - tmpfs tmpfs rw,seclabel' source: 'tmpfs' source: 'tmpfs' source: 'tmpfs' expected: 'tmpfs' target: '/tmp/foo bar' target: '/tmp/foo\rbar' target: '/tmp/foo bar' expected: '/tmp/foo bar'

The test added in previous commit shows that libmount does the unescaping internally.

keszybz · 2019-04-09T07:08:35Z

Assertion 'r == 0' failed at ../src/test/test-libmount.c:43, function test_libmount_unescaping_one(). Aborting.

Argh, it doesn't want to parse the line with empty source at all. I'll update the test once more.

keszybz · 2019-04-09T09:18:06Z

bionic-i386: "FAILED TESTS: test/TEST-30-ONCLOCKCHANGE" — unrelated.

evverx · 2019-04-13T12:14:14Z

The test fails under Valgrind and ASan when libmount-2.32.1-1.fc29 is used:

==8653==
==8653== HEAP SUMMARY:
==8653==     in use at exit: 208 bytes in 1 blocks
==8653==   total heap usage: 111 allocs, 110 frees, 54,807 bytes allocated
==8653==
==8653== 208 bytes in 1 blocks are definitely lost in loss record 1 of 1
==8653==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
==8653==    by 0x4B97336: mnt_new_fs (in /usr/lib64/libmount.so.1.1.0)
==8653==    by 0x4BA1F6C: mnt_table_parse_stream (in /usr/lib64/libmount.so.1.1.0)
==8653==    by 0x4014C5: test_libmount_unescaping_one (test-libmount.c:30)
==8653==    by 0x401E65: test_libmount_unescaping (test-libmount.c:93)
==8653==    by 0x401EA5: main (test-libmount.c:116)
==8653==
==8653== LEAK SUMMARY:
==8653==    definitely lost: 208 bytes in 1 blocks
==8653==    indirectly lost: 0 bytes in 0 blocks
==8653==      possibly lost: 0 bytes in 0 blocks
==8653==    still reachable: 0 bytes in 0 blocks
==8653==         suppressed: 0 bytes in 0 blocks
==8653==
==8653== For counts of detected and suppressed errors, rerun with: -v
==8653== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Other that that I'm wondering why the test for libmount should be kept in the repository? Wouldn't it be better to keep it there? cc @karelzak

evverx · 2019-04-13T12:42:34Z

Wouldn't it be better to keep it there?

It probably would if util-linux was built with ASan but looking at https://travis-ci.org/karelzak/util-linux/builds/518734784?utm_source=github_status&utm_medium=notification I can't find anything like that. @karelzak would it be possible to also compile util-linux with ASan on Travis CI? Judging by #8504, it doesn't seem to be the first time memory leaks in libmount have been discovered.

karelzak · 2019-04-15T09:58:13Z

I'll think about it. We have --enable-asan, but it's unused on travis for now. Need to test it a little bit.

karelzak · 2019-04-15T10:13:33Z

BTW, where in the test is mnt_unref_table() ?

Anyway, we have regression test for both use cases in upstream tree:
empty source: https://github.com/karelzak/util-linux/blob/master/tests/ts/libmount/files/mountinfo_nosrc
\r in path: https://github.com/karelzak/util-linux/blob/master/tests/ts/libmount/files/mountinfo

poettering · 2019-04-15T10:43:12Z

@karelzak we use gcc's cleanup attribute heavily in systemd, i.e. _cleanup_(mnt_free_tablep) is applied to the table variable, which means the table is freed automatically when it leaves scope.

karelzak · 2019-04-15T10:58:05Z

@karelzak we use gcc's cleanup attribute heavily in systemd, i.e. _cleanup_(mnt_free_tablep) is applied to the table variable, which means the table is freed automatically when it leaves scope.

Ah, I've read only end of the function. Sorry.

Addresses: systemd/systemd#12252 Signed-off-by: Karel Zak <[email protected]>

Addresses: Addresses: systemd/systemd#12252 Signed-off-by: Karel Zak <[email protected]>

The fuzzer is supposed to cover `mnt_table_parse_stream`, which is used by systemd to parse /proc/self/mountinfo. The systemd project has run into memory leaks there at least twice: systemd/systemd#12252 (comment) systemd/systemd#8504 so it seems to be a good idea to continuously fuzz that particular function. The patch can be tested locally by installing clang and running ./tools/oss-fuzz.sh. Currently the fuzzer is failing with ``` ================================================================= ==96638==ERROR: LeakSanitizer: detected memory leaks Direct leak of 216 byte(s) in 1 object(s) allocated from: #0 0x50cd77 in calloc (/home/vagrant/util-linux/out/test_mount_fuzz+0x50cd77) #1 0x58716a in mnt_new_fs /home/vagrant/util-linux/libmount/src/fs.c:36:25 #2 0x54f224 in __table_parse_stream /home/vagrant/util-linux/libmount/src/tab_parse.c:728:9 #3 0x54eed8 in mnt_table_parse_stream /home/vagrant/util-linux/libmount/src/tab_parse.c:804:8 #4 0x5448b2 in LLVMFuzzerTestOneInput /home/vagrant/util-linux/libmount/src/fuzz.c:19:16 util-linux#5 0x44cc88 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/vagrant/util-linux/out/test_mount_fuzz+0x44cc88) util-linux#6 0x44d8b0 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/home/vagrant/util-linux/out/test_mount_fuzz+0x44d8b0) util-linux#7 0x44e270 in fuzzer::Fuzzer::MutateAndTestOne() (/home/vagrant/util-linux/out/test_mount_fuzz+0x44e270) util-linux#8 0x450617 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/home/vagrant/util-linux/out/test_mount_fuzz+0x450617) util-linux#9 0x43adbb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/vagrant/util-linux/out/test_mount_fuzz+0x43adbb) util-linux#10 0x42ad46 in main (/home/vagrant/util-linux/out/test_mount_fuzz+0x42ad46) util-linux#11 0x7fa084f621a2 in __libc_start_main (/lib64/libc.so.6+0x271a2) SUMMARY: AddressSanitizer: 216 byte(s) leaked in 1 allocation(s). INFO: to ignore leaks on libFuzzer side use -detect_leaks=0. ``` Once the bug is fixed and the OSS-Fuzz counterpart is merged it should be possible to turn on CIFuzz to make sure the fuzz target can be built and run for some time without crashing: https://google.github.io/oss-fuzz/getting-started/continuous-integration/

The fuzzer is supposed to cover `mnt_table_parse_stream`, which is used by systemd to parse /proc/self/mountinfo. The systemd project has run into memory leaks there at least twice: systemd/systemd#12252 (comment) systemd/systemd#8504 so it seems to be a good idea to continuously fuzz that particular function. The patch can be tested locally by installing clang and running ./tools/oss-fuzz.sh. Currently the fuzzer is failing with ``` ================================================================= ==96638==ERROR: LeakSanitizer: detected memory leaks Direct leak of 216 byte(s) in 1 object(s) allocated from: #0 0x50cd77 in calloc (/home/vagrant/util-linux/out/test_mount_fuzz+0x50cd77) #1 0x58716a in mnt_new_fs /home/vagrant/util-linux/libmount/src/fs.c:36:25 #2 0x54f224 in __table_parse_stream /home/vagrant/util-linux/libmount/src/tab_parse.c:728:9 #3 0x54eed8 in mnt_table_parse_stream /home/vagrant/util-linux/libmount/src/tab_parse.c:804:8 #4 0x5448b2 in LLVMFuzzerTestOneInput /home/vagrant/util-linux/libmount/src/fuzz.c:19:16 util-linux#5 0x44cc88 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/vagrant/util-linux/out/test_mount_fuzz+0x44cc88) util-linux#6 0x44d8b0 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/home/vagrant/util-linux/out/test_mount_fuzz+0x44d8b0) util-linux#7 0x44e270 in fuzzer::Fuzzer::MutateAndTestOne() (/home/vagrant/util-linux/out/test_mount_fuzz+0x44e270) util-linux#8 0x450617 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/home/vagrant/util-linux/out/test_mount_fuzz+0x450617) util-linux#9 0x43adbb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/vagrant/util-linux/out/test_mount_fuzz+0x43adbb) util-linux#10 0x42ad46 in main (/home/vagrant/util-linux/out/test_mount_fuzz+0x42ad46) util-linux#11 0x7fa084f621a2 in __libc_start_main (/lib64/libc.so.6+0x271a2) SUMMARY: AddressSanitizer: 216 byte(s) leaked in 1 allocation(s). INFO: to ignore leaks on libFuzzer side use -detect_leaks=0. ``` Once the bug is fixed and the OSS-Fuzz counterpart is merged it should be possible to turn on CIFuzz to make sure the fuzz target can be built and run for some time without crashing: https://google.github.io/oss-fuzz/getting-started/continuous-integration/ Signed-off-by: Evgeny Vereshchagin <[email protected]>

keszybz added the pid1 label Apr 8, 2019

keszybz added this to the v242 milestone Apr 8, 2019

poettering added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Apr 8, 2019

keszybz force-pushed the libmount-dont-unescape branch from 9836665 to 53b1fc9 Compare April 8, 2019 21:13

keszybz added 2 commits April 9, 2019 09:07

pid1,shutdown: do not cunescape paths from libmount

9d1b2b2

The test added in previous commit shows that libmount does the unescaping internally.

keszybz force-pushed the libmount-dont-unescape branch from 53b1fc9 to 9d1b2b2 Compare April 9, 2019 07:09

keszybz added ci-failure-appears-unrelated and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Apr 9, 2019

keszybz merged commit 74b4588 into systemd:master Apr 9, 2019

keszybz deleted the libmount-dont-unescape branch April 9, 2019 10:09

karelzak added a commit to util-linux/util-linux that referenced this pull request Apr 15, 2019

libmount: fix memleak on parse errors

82e3947

Addresses: systemd/systemd#12252 Signed-off-by: Karel Zak <[email protected]>

karelzak added a commit to util-linux/util-linux that referenced this pull request Apr 15, 2019

build-sys: enable ASAN on travis-ci

7bd44e8

Addresses: Addresses: systemd/systemd#12252 Signed-off-by: Karel Zak <[email protected]>

evverx mentioned this pull request May 3, 2019

travis: run the unit tests under ASan/UBSan on Ubuntu Xenial #12471

Closed

evverx mentioned this pull request Feb 12, 2020

travis: add aarch64, s390x, and ppc64le jobs #13785

Closed

evverx mentioned this pull request Jun 25, 2020

add a fuzzer for mnt_table_parse_stream util-linux/util-linux#1068

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Don't unescape paths from libmount#12252

Don't unescape paths from libmount#12252
keszybz merged 3 commits intosystemd:masterfrom
keszybz:libmount-dont-unescape

keszybz commented Apr 8, 2019

Uh oh!

keszybz commented Apr 8, 2019

Uh oh!

keszybz commented Apr 9, 2019

Uh oh!

keszybz commented Apr 9, 2019

Uh oh!

evverx commented Apr 13, 2019

Uh oh!

evverx commented Apr 13, 2019 •

edited

Loading

Uh oh!

karelzak commented Apr 15, 2019

Uh oh!

karelzak commented Apr 15, 2019

Uh oh!

poettering commented Apr 15, 2019

Uh oh!

karelzak commented Apr 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

keszybz commented Apr 8, 2019

Uh oh!

keszybz commented Apr 8, 2019

Uh oh!

keszybz commented Apr 9, 2019

Uh oh!

keszybz commented Apr 9, 2019

Uh oh!

evverx commented Apr 13, 2019

Uh oh!

evverx commented Apr 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karelzak commented Apr 15, 2019

Uh oh!

karelzak commented Apr 15, 2019

Uh oh!

poettering commented Apr 15, 2019

Uh oh!

karelzak commented Apr 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

evverx commented Apr 13, 2019 •

edited

Loading