Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code cleanup & rest of the "systemd-dfuzzer" patches #24

Merged
merged 13 commits into from
Apr 24, 2022

Conversation

mrc0mmand
Copy link
Member

This PR collects all the remaining patches from systemd-dfuzzer (mainly this open PR: systemd/systemd-dfuzzer#10) and also addresses the -Werror issue found by @evverx in https://github.com/matusmarhefka/dfuzzer/pull/16#discussion_r850082248, where all warnings were suppressed by the -w option.

One commit I left out is systemd/systemd-dfuzzer@58ae664, since I'm not sure how relevant the package-info parsing stuff is, hence the last "debug only" commit, since this function is still emitting a compile-time warning.

@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2022

This pull request introduces 1 alert when merging 6972454 into e54710a - view on LGTM.com

new alerts:

  • 1 for Uncontrolled data used in OS command

@evverx
Copy link
Member

evverx commented Apr 23, 2022

@mrc0mmand thanks! Could you include https://github.com/matusmarhefka/dfuzzer/pull/22 just to make sure that the package is more or less intact. Another option would be to merge that PR and rebase this PR on top of the master branch. I'll take a closer look a bit later today.

@evverx
Copy link
Member

evverx commented Apr 23, 2022

One commit I left out is systemd/systemd-dfuzzer@58ae664, since I'm not sure how relevant the package-info parsing stuff is

I think it would make sense to keep it only if https://github.com/matusmarhefka/dfuzzer/issues/21 was resolved somehow because it should probably make it easier to report bugs like https://github.com/matusmarhefka/dfuzzer/issues/20. In that particular case it looked like

[PACKAGE: avahi-0.8-14.fc35.x86_64
...

Having said that as far as I can remember it failed under Valgrind and didn't add trailing square brackets so it needs polishing anyway.

@evverx
Copy link
Member

evverx commented Apr 23, 2022

FWIW clang is still complaining:

dfuzzer.c:575:21: error: comparison of array 'df_suppression' not equal to a null pointer is always true [-Werror,-Wtautological-pointer-compare]
                if (df_suppression != NULL) {
                    ^~~~~~~~~~~~~~    ~~~~
1 error generated.

but I think it has nothing to do with this PR.

@evverx
Copy link
Member

evverx commented Apr 23, 2022

One commit I left out is systemd/systemd-dfuzzer@58ae664, since I'm not sure how relevant the package-info parsing stuff is

Thinking about it a bit more I think it should be removed because I'm not sure it can cover "edge" cases (or less popular distributions). Also I agree with LGTM that running commands based on the contents of /proc/%d/cmdline isn't exactly the best practice.

@evverx evverx mentioned this pull request Apr 23, 2022
Closed
// Initializes the type system.
g_type_init();
rses = df_process_bus(G_BUS_TYPE_SESSION);
rsys = df_process_bus(G_BUS_TYPE_SYSTEM);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's neat! I think in terms of testing systemd it would be even better if it was possible to connect to just one bus so as not to poke PID1 twice in systemd/systemd#22547 but I think that can wait because without void methods dfuzzer is relatively fast anyway.

@mrc0mmand mrc0mmand force-pushed the code-cleanup branch 4 times, most recently from d562780 to 088b787 Compare April 23, 2022 15:32
@mrc0mmand
Copy link
Member Author

mrc0mmand commented Apr 23, 2022

One commit I left out is systemd/systemd-dfuzzer@58ae664, since I'm not sure how relevant the package-info parsing stuff is

Thinking about it a bit more I think it should be removed because I'm not sure it can cover "edge" cases (or less popular distributions). Also I agree with LGTM that running commands based on the contents of /proc/%d/cmdline isn't exactly the best practice.

Definitely agree! I backported the patch as well and dropped the "debug only" stuff which shouldn't be necessary thanks to that.

FWIW clang is still complaining:

dfuzzer.c:575:21: error: comparison of array 'df_suppression' not equal to a null pointer is always true [-Werror,-Wtautological-pointer-compare]
                if (df_suppression != NULL) {
                    ^~~~~~~~~~~~~~    ~~~~
1 error generated.

but I think it has nothing to do with this PR.

Whoops, that one slipped past me (as well as a couple of other warnings caused by -Wextra), I guess adding clang into the CI matrix would be a good idea.

@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2022

This pull request fixes 1 alert when merging 088b787 into e54710a - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@evverx
Copy link
Member

evverx commented Apr 23, 2022

I guess adding clang into the CI matrix would be a good idea.

It's kind of complicated I think. With -Werror it fails to compile with

dfuzzer.c:22:10: fatal error: 'gio/gio.h' file not found
#include <gio/gio.h>
         ^~~~~~~~~~~
1 error generated.
fuzz.c:21:10: fatal error: 'gio/gio.h' file not found
#include <gio/gio.h>
         ^~~~~~~~~~~
1 error generated.
introspection.c:20:10: fatal error: 'gio/gio.h' file not found
#include <gio/gio.h>
         ^~~~~~~~~~~
1 error generated.
rand.c:20:10: fatal error: 'gio/gio.h' file not found
#include <gio/gio.h>
         ^~~~~~~~~~~
1 error generated.

because CFLAGS aren't passed to clang -MM but if they are passed there iclang complains about unused command line arguments

clang-13: error: -lgio-2.0: 'linker' input unused [-Werror,-Wunused-command-line-argument]
clang-13: error: -lgobject-2.0: 'linker' input unused [-Werror,-Wunused-command-line-argument]
clang-13: error: -lglib-2.0: 'linker' input unused [-Werror,-Wunused-command-line-argument]
clang-13: error: -lffi: 'linker' input unused [-Werror,-Wunused-command-line-argument]

Ideally the -l flags should be moved to LDFLAGS and passed to the linker only but that's certainly going to break the package so I'd just ignore it for now.

@mrc0mmand
Copy link
Member Author

@evverx I think the last commit should fix your Packit woes, since, as I found out just now, CFLAGS=xxx make and make CFLAGS=xxx are not the same, and in the latter case the CFLAGS= definition in the Makefile is ignored, hence the fails in Packit, see: https://www.gnu.org/software/make/manual/make.html#Override-Directive.

@lgtm-com

This comment was marked as resolved.

@evverx
Copy link
Member

evverx commented Apr 23, 2022

the latter case the CFLAGS= definition in the Makefile is ignored

Currently the specfile assumes that it can override CFLAGS as far as I can see so that patch should certainly be tested with Packit.

I think Packit failed because dfuzzer failed to compile on i386 which I fixed with

--- a/src/fuzz.c
+++ b/src/fuzz.c
@@ -344,15 +344,15 @@ static int df_fuzz_write_log(void)
                                         ;
                                         gint64 tmp6;
                                         g_variant_get(s->var, s->sig, &tmp6);
-                                        df_fail("-- '%ld'\n", tmp6);
-                                        FULL_LOG("%ld;", tmp6);
+                                        df_fail("-- '%" G_GINT64_FORMAT "'\n", tmp6);
+                                        FULL_LOG("%" G_GINT64_FORMAT, tmp6);
                                         break;
                                 case 't':
                                         ;
                                         guint64 tmp7;
                                         g_variant_get(s->var, s->sig, &tmp7);
-                                        df_fail("-- '%lu'\n", tmp7);
-                                        FULL_LOG("%lu;", tmp7);
+                                        df_fail("-- '%" G_GUINT64_FORMAT "'\n", tmp7);
+                                        FULL_LOG("%" G_GUINT64_FORMAT, tmp7);
                                         break;

and I also added __attribute__((__format__(printf, 1, 2))) to df_{debug,verbose,fail} just in case.

@evverx evverx mentioned this pull request Apr 23, 2022
@evverx
Copy link
Member

evverx commented Apr 23, 2022

@mrc0mmand could you force-push the PR to trigger Packit?

@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2022

This pull request fixes 1 alert when merging eb2abb3 into 6a23217 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@evverx
Copy link
Member

evverx commented Apr 23, 2022

Looks like it didn't break the package but it seems most arguments are passed twice. I think at some point the package should stop passing pkg-config --cflags --libs gio-2.0 libffi and let the build system handle it. I don't think Makefile should add stuff like FORTIFY_SOURCE by default but I'd tweak it later because even without that there are a lot of moving parts here.

@mrc0mmand
Copy link
Member Author

Looks like it didn't break the package but it seems most arguments are passed twice. I think at some point the package should stop passing pkg-config --cflags --libs gio-2.0 libffi and let the build system handle it. I don't think Makefile should add stuff like FORTIFY_SOURCE by default but I'd tweak it later because even without that there are a lot of moving parts here.

Yeah, the Makefile and related stuff really needs some love, but also a thorough testing to not break anything.

@evverx
Copy link
Member

evverx commented Apr 23, 2022

Looks like the exit code changed somehow. With this patch applied dfuzzer returns 4:

./dfuzzer -v -n org.freedesktop.Avahi
...
Exit status: 4

and without this patch it returns 2:

dfuzzer -v -n org.freedesktop.Avahi
...
Exit status: 2

Given that there is a bug in avahi I think 2 should be the right exit code (judging by the comments there)

@mrc0mmand
Copy link
Member Author

Looks like the exit code changed somehow. With this patch applied dfuzzer returns 4:

./dfuzzer -v -n org.freedesktop.Avahi
...
Exit status: 4

and without this patch it returns 2:

dfuzzer -v -n org.freedesktop.Avahi
...
Exit status: 2

Given that there is a bug in avahi I think 2 should be the right exit code (judging by the comments there)

Wow, thank you, I managed to miss onetwo return values while refactoring, hopefully fixed.

@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2022

This pull request fixes 1 alert when merging de6bc9a into 6a23217 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@evverx
Copy link
Member

evverx commented Apr 23, 2022

Thanks! I'll take a look a bit later. FWIW dfuzzer is on Coverity now: https://scan.coverity.com/projects/dfuzzer.

@@ -88,7 +88,10 @@ guint64 df_rand_guint64(void);
* @return Generated pseudo-random double precision floating point number
* from interval <0, 1>
*/
inline double drand(void);
inline double drand(void)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be static inline. Without static it fails to compile with -O0 with

cc -Wall -Wno-unused-parameter -O0 -fstack-protector --param=ssp-buffer-size=4 `pkg-config --cflags --libs gio-2.0 libffi` -g   -c -o fuzz.o fuzz.c
cc -Wall -Wno-unused-parameter -O0 -fstack-protector --param=ssp-buffer-size=4 `pkg-config --cflags --libs gio-2.0 libffi` -g   -c -o rand.o rand.c
cc dfuzzer.o introspection.o fuzz.o rand.o util.o -Wall -Wno-unused-parameter -O0 -fstack-protector --param=ssp-buffer-size=4 `pkg-config --cflags --libs gio-2.0 libffi` -g -o dfuzzer
/usr/bin/ld: rand.o: in function `df_rand_gdouble':
/home/vagrant/dfuzzer/src/rand.c:357: undefined reference to `drand'
collect2: error: ld returned 1 exit status
make: *** [Makefile:18: dfuzzer] Error 1

I discovered it with meson.build:

project('dfuzzer', 'c',
        version : '1.4',
        default_options: [
                'c_std=gnu11',
        ],
)

libgio = dependency('gio-2.0', required : true)
libffi = dependency('libffi', required : true)

dfuzzer_sources = files(
        'dfuzzer.c',
        'dfuzzer.h',
        'introspection.c',
        'introspection.h',
        'fuzz.c',
        'fuzz.h',
        'rand.c',
        'rand.h',
        'util.c',
        'util.h',
)

executable(
        'dfuzzer',
        dfuzzer_sources,
        dependencies : [libgio, libffi],
        install : true
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has nothing to do with this patch though. I'll fix it in https://github.com/matusmarhefka/dfuzzer/pull/26

evverx referenced this pull request in evverx/dfuzzer Apr 23, 2022
inspired by https://github.com/matusmarhefka/dfuzzer/pull/24#issuecomment-1107522921

With this patch applied dfuzzer can be built with clang with
```
CC=clang meson build
ninja -C ./build -v
```
evverx referenced this pull request in evverx/dfuzzer Apr 23, 2022
inspired by https://github.com/matusmarhefka/dfuzzer/pull/24#issuecomment-1107522921

With this patch applied dfuzzer can be built with clang with
```
CC=clang meson build
ninja -C ./build -v
```
evverx referenced this pull request in evverx/dfuzzer Apr 24, 2022
inspired by https://github.com/matusmarhefka/dfuzzer/pull/24#issuecomment-1107522921

With this patch applied dfuzzer can be built with clang with
```
CC=clang meson build
ninja -C ./build -v
```
mkdir dfuzzer-logs
dfuzzer --log-dir dfuzzer-logs -v -n org.freedesktop.systemd1
# Test a non-existent bus
if sudo dfuzzer --log-dir "" --bus this.should.not.exist; then false; fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I'm not sure --log-dir should accept empty strings. When dfuzzer is run as root it seems to pollute the root directory for no apparent reason. It has always worked this way though as far as I understand so it can be addressed later.

I'll go ahead and merge the PR. Once it lands I'll send dfuzzer to coverity. Controversial patches can always be reverted/revisited I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once it lands I'll send dfuzzer to coverity

I've just opened https://github.com/matusmarhefka/dfuzzer/issues/27

@evverx evverx merged commit 694a000 into dbus-fuzzer:master Apr 24, 2022
@mrc0mmand mrc0mmand deleted the code-cleanup branch April 24, 2022 11:17
evverx referenced this pull request in evverx/dfuzzer Apr 24, 2022
inspired by https://github.com/matusmarhefka/dfuzzer/pull/24#issuecomment-1107522921

With this patch applied dfuzzer can be built with clang with
```
CC=clang meson build
ninja -C ./build -v
```
evverx referenced this pull request in evverx/dfuzzer Apr 24, 2022
inspired by https://github.com/matusmarhefka/dfuzzer/pull/24#issuecomment-1107522921

With this patch applied dfuzzer can be built with clang with
```
CC=clang meson build
ninja -C ./build -v
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants