strophe / libstrophe

A simple, lightweight C library for writing XMPP clients
http://strophe.im/libstrophe
Other
401 stars 163 forks source link

Add basic fuzzing support for libstrophe #179

Closed JordyZomer closed 3 years ago

JordyZomer commented 3 years ago

Hi!

Based on https://github.com/profanity-im/profanity/issues/1509#issuecomment-858110643 I added some fuzzing support to libstrophe.

I added the fuzzer to the TESTS in the Makefile after the check_PROGRAMS because you don't want to run this in your test-driver, this would cause your tests to wait until the fuzzer caused a crash. Therefore I added this later.

This compiles perfectly for me and I can run the fuzzer with:

libstrophe# ./tests/test_fuzz
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2708216363
INFO: Loaded 1 modules   (4 inline 8-bit counters): 4 [0x5c4440, 0x5c4444),
INFO: Loaded 1 PC tables (4 PCs): 4 [0x583610,0x583650),
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
#2      INITED cov: 1 ft: 1 corp: 1/1b exec/s: 0 rss: 32Mb

Best Regards,

Jordy Zomer

JordyZomer commented 3 years ago

Trying to fix the CI issues, due to GCC not having fsanitize=fuzzer, I changed the default CC to clang because you already rely on clang-format I thought this would be an okay assumption to make. Please let me know if you have any better suggestions :)

sjaeckel commented 3 years ago

you can try to add the following to the .travis.yml

compiler:
  - clang
JordyZomer commented 3 years ago

Thanks @sjaeckel let's see if this works :)

jubalh commented 3 years ago

Fails with:

tests/test_fuzz.c:11:32: error: unused parameter 'name'

      [-Werror,-Wunused-parameter]

void cbtest_handle_start(char *name, char **attrs, void *userdata) { (vo...

                               ^

tests/test_fuzz.c:13:30: error: unused parameter 'name'

      [-Werror,-Wunused-parameter]

void cbtest_handle_end(char *name, void *userdata) { (void)userdata; }

                             ^

tests/test_fuzz.c:15:42: error: unused parameter 'stanza'

      [-Werror,-Wunused-parameter]

void cbtest_handle_stanza(xmpp_stanza_t *stanza, void *userdata) { (void...
JordyZomer commented 3 years ago

@jubalh Thanks I fixed it :)

jubalh commented 3 years ago

@JordyZomer great! Now I would just squash the fix commits together.

JordyZomer commented 3 years ago

@jubalh should be done! If it's not done correctly please let me know, haven't been using these git features in ages ;)

jubalh commented 3 years ago

Not done correctly :) Now we have a merge commit instead :) git rebase -i HEAD~NR where nr is the commits you want to rebase. You can then squash, move them around edit etc.

sjaeckel commented 3 years ago

You didn't squash the commits, you only merged some old commit back.

There are multiple ways to do this now ;)

Easiest would be to soft-reset to strophe/master and create a new commit

# that's my setup: remote "jordy" points to your fork, "origin" points to the original libstrophe
$ git remote -v
jordy   https://github.com/JordyZomer/libstrophe (fetch)
...
origin  https://github.com/strophe/libstrophe.git (fetch)
...
# all of this assumes that you're still on master which points to jordy/master
$ git describe --always     
28da39e
$ git branch --show-current
master
# reset your master to point to the original master, all changes will be kept
$ git reset origin/master
# now commit everything into one commit and force-push your branch
$ git push --force

An alternative would be to do an interactive rebase where you mark all the commits after the first as fixup so they automatically get squashed. After this you also have to force-push.

$ git rebase --interactive origin/master
JordyZomer commented 3 years ago

@jubalh @sjaeckel Thanks! Managed to fix it :)

JordyZomer commented 3 years ago

@jubalh any updates on when/if this is going to be merged? :D

jubalh commented 3 years ago

@JordyZomer depends when @pasis has time :)

pasis commented 3 years ago

Sorry for the delay.

@JordyZomer please fix coding style in tests/test_fuzz.c (you can run make format and git add tests/test_fuzz.c, but don't include style fixes for other files). You can also check this output: https://travis-ci.org/github/strophe/libstrophe/jobs/774541180 .

As for patch itself, it sounds useful and I'd like to merge it. However, I'm concerned about AC_PROG_CC([clang]). We don't force any specific compiler. libstrophe is used on embedded systems, in game consoles, exotic and old OSes etc. Clang may not be present on all platforms. It is fine to force clang in travis config for now, but in the future I'd prefer to have autodetection of compiler and enable fuzzer test only for clang. And we can run tests for both gcc and clang with travis.

Could you revert change in configure.ac please?

JordyZomer commented 3 years ago

Hi @pasis!

I formatted the fuzzer, reverted the changes in .travis.yml and configure.ac then added a --enable-fuzzing option to the configure. When you enable this the fuzzers will be built. If you do this without setting CC to clang you'll get an error.

Hope this is better :)

Jordy

pasis commented 3 years ago

Looks good to me, further improvements can be done after merging. Thanks.

pasis commented 3 years ago

Merged, thanks. This test found a memory leak when parser is fed with a malformed stanza with depth more than 1. The fix will be in master soon if you're interested in details.