Add c-lightning module for differential fuzzing - DRAFT by erickcestari · Pull Request #132 · bitcoinfuzz/bitcoinfuzz

erickcestari · 2025-04-06T16:15:11Z

This commit introduces a c-lightning module to the bitcoinfuzz project, enabling differential fuzzing of lightning invoices.

Key changes:

Module Integration:

Add modules/clightning directory with module implementation and wrapper
Implement c-lightning library wrapper for invoice parsing

Build System:

Update GitHub workflow to include c-lightning module in fuzzing
Modify Makefile to handle c-lightning dependencies

Differential Fuzzing:

Enhance driver to compare invoice deserialization across implementations
Enable detection of discrepancies between c-lightning and other modules

Dependencies:

Add c-lightning as a submodule in external/lightning
Configure necessary build dependencies

Closes #96

yuvicc · 2025-04-07T05:44:06Z

Concept ACK!

README.md

brunoerg · 2025-04-07T16:38:34Z

I tried to build this module and got:

ar libccan.a
ar: creating archive libccan.a
ld ccan/ccan/cdump/tools/cdump-enumstr
ccan/ccan/cdump/tools/cdump-enumstr common/htlc_state.h > common/htlc_state_names_gen.h
wiregen common/status_wiregen.h
Traceback (most recent call last):
  File "/Users/brunogarcia/projects/bitcoinfuzz/external/lightning/tools/generate-wire.py", line 27, in <module>
    from mako.template import Template
ModuleNotFoundError: No module named 'mako'
make[1]: *** [common/status_wiregen.h] Error 1
rm external/build-arm64-apple-darwin23.3.0/libwally-core-build/src/secp256k1/libsecp256k1.la
make: *** [libclightning_wrapper.a] Error 2

brunoerg · 2025-04-07T16:50:34Z

I installed mako and now I get the following error:

ar rc external/build-arm64-apple-darwin23.3.0/libjsmn.a external/build-arm64-apple-darwin23.3.0/jsmn-build/jsmn.o
ar libccan.a
ld ccan/ccan/cdump/tools/cdump-enumstr
ccan/ccan/cdump/tools/cdump-enumstr common/htlc_state.h > common/htlc_state_names_gen.h
cc clightning_lib.c
clightning_lib.c:52:57: error: format specifies type 'long' but the argument has type 'u64' (aka 'unsigned long long') [-Werror,-Wformat]
   52 |         snprintf(amount_str, sizeof(amount_str), "%ld", invoice->msat->millisatoshis);
      |                                                   ~~~   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                   %llu
/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk/usr/include/secure/_stdio.h:57:62: note: expanded from macro 'snprintf'
   57 |   __builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
      |                                                              ^~~~~~~~~~~
clightning_lib.c:80:53: error: format specifies type 'long' but the argument has type 'u64' (aka 'unsigned long long') [-Werror,-Wformat]
   80 |     snprintf(expiry_str, sizeof(expiry_str), "%ld", invoice->expiry);
      |                                               ~~~   ^~~~~~~~~~~~~~~
      |                                               %llu
/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk/usr/include/secure/_stdio.h:57:62: note: expanded from macro 'snprintf'
   57 |   __builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
      |                                                              ^~~~~~~~~~~
clightning_lib.c:86:59: error: format specifies type 'long' but the argument has type 'u64' (aka 'unsigned long long') [-Werror,-Wformat]
   86 |     snprintf(timestamp_str, sizeof(timestamp_str), "%ld", invoice->timestamp);
      |                                                     ~~~   ^~~~~~~~~~~~~~~~~~
      |                                                     %llu
/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk/usr/include/secure/_stdio.h:57:62: note: expanded from macro 'snprintf'
   57 |   __builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
      |                                                              ^~~~~~~~~~~
3 errors generated.
make[1]: *** [clightning_lib.o] Error 1
make: *** [libclightning_wrapper.a] Error 2

erickcestari · 2025-04-07T16:54:25Z

Yes, I forgot. Python and the Mako library are required to build c-lightning.

I tried to build this module and got:

ar libccan.a
ar: creating archive libccan.a
ld ccan/ccan/cdump/tools/cdump-enumstr
ccan/ccan/cdump/tools/cdump-enumstr common/htlc_state.h > common/htlc_state_names_gen.h
wiregen common/status_wiregen.h
Traceback (most recent call last):
  File "/Users/brunogarcia/projects/bitcoinfuzz/external/lightning/tools/generate-wire.py", line 27, in <module>
    from mako.template import Template
ModuleNotFoundError: No module named 'mako'
make[1]: *** [common/status_wiregen.h] Error 1
rm external/build-arm64-apple-darwin23.3.0/libwally-core-build/src/secp256k1/libsecp256k1.la
make: *** [libclightning_wrapper.a] Error 2

modules/clightning/Makefile

erickcestari · 2025-04-07T17:18:04Z

ar rc external/build-arm64-apple-darwin23.3.0/libjsmn.a external/build-arm64-apple-darwin23.3.0/jsmn-build/jsmn.o
ar libccan.a
ld ccan/ccan/cdump/tools/cdump-enumstr
ccan/ccan/cdump/tools/cdump-enumstr common/htlc_state.h > common/htlc_state_names_gen.h
cc clightning_lib.c
clightning_lib.c:52:57: error: format specifies type 'long' but the argument has type 'u64' (aka 'unsigned long long') [-Werror,-Wformat]
   52 |         snprintf(amount_str, sizeof(amount_str), "%ld", invoice->msat->millisatoshis);
      |                                                   ~~~   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                   %llu
/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk/usr/include/secure/_stdio.h:57:62: note: expanded from macro 'snprintf'
   57 |   __builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
      |                                                              ^~~~~~~~~~~
clightning_lib.c:80:53: error: format specifies type 'long' but the argument has type 'u64' (aka 'unsigned long long') [-Werror,-Wformat]
   80 |     snprintf(expiry_str, sizeof(expiry_str), "%ld", invoice->expiry);
      |                                               ~~~   ^~~~~~~~~~~~~~~
      |                                               %llu
/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk/usr/include/secure/_stdio.h:57:62: note: expanded from macro 'snprintf'
   57 |   __builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
      |                                                              ^~~~~~~~~~~
clightning_lib.c:86:59: error: format specifies type 'long' but the argument has type 'u64' (aka 'unsigned long long') [-Werror,-Wformat]
   86 |     snprintf(timestamp_str, sizeof(timestamp_str), "%ld", invoice->timestamp);
      |                                                     ~~~   ^~~~~~~~~~~~~~~~~~
      |                                                     %llu
/Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk/usr/include/secure/_stdio.h:57:62: note: expanded from macro 'snprintf'
   57 |   __builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
      |                                                              ^~~~~~~~~~~
3 errors generated.
make[1]: *** [clightning_lib.o] Error 1
make: *** [libclightning_wrapper.a] Error 2

The issue was fixed by replacing the incorrect %ld format specifiers with "%" PRIu64 (from <inttypes.h>) to properly handle u64 values on both macOS (where it's defined as unsigned long long) and Linux (where it's defined as long unsigned int).

yuvicc

Tested ACK 0306c99

Thanks for the PR

Tested on Ubuntu and works file
No memory leaks possible

I Will be running the target to see if there's any crash.

modules/clightning/clightning_lib.c

morehouse · 2025-04-09T14:59:27Z

modules/clightning/clightning_lib.h

+#ifdef __cplusplus
+/* C++ specific includes */
+#include <cstdint>
+extern "C" {
+#else
+/* C specific includes */
+#include <stdint.h>
+#include <stdbool.h>
+#endif


Why is the C++-conditional compilation needed? Shouldn't we always be compiling it one way (either as C or as C++)?

Because the header is included in both C and C++ code. extern "C" is required when including C headers in C++ code to prevent name mangling, which ensures the C++ code can link properly to the C-compiled libclightning_wrapper.a library.

morehouse · 2025-04-09T15:03:22Z

modules/clightning/clightning_lib.c

+    return hex;
+}
+
+char* clightning_des_invoice(const char* input) {


It would be nice to use C++ for this.

Is it possible to compile CLN as a library that we link against instead of building this into CLN itself?

The CLN Makefile doesn't build a static library by default. We'll likely need to implement a workaround to generate one. Instead of copying and pasting the entire Makefile, we could simply append custom rules at the end to handle the static library build.

If the library approach does end up being cleaner, maybe CLN would accept a patch to their Makefile upstream, so we don't have to append rules here.

morehouse · 2025-04-09T15:04:28Z

modules/clightning/Makefile-clightning

+CLIGHTNING_OBJS = \
+	clightning_lib.o
+
+$(LIB_NAME): $(CLIGHTNING_OBJS) libccan.a $(BITCOIN_OBJS) $(COMMON_OBJS) $(WIRE_OBJS) $(EXTERNAL_LIBS)


Having a copy-paste of CLN's Makefile with this one addition is kinda ugly, and might break in the future.

It would be nice if we could just link against CLN instead.

This commit introduces a c-lightning module to the bitcoinfuzz project, enabling differential fuzzing of lightning invoices. Key changes: Module Integration: - Add `modules/clightning` directory with module implementation and wrapper - Implement c-lightning library wrapper for invoice parsing Build System: - Update GitHub workflow to include c-lightning module in fuzzing - Modify Makefile to handle c-lightning dependencies Differential Fuzzing: - Enhance driver to compare invoice deserialization across implementations - Enable detection of discrepancies between c-lightning and other modules Dependencies: - Add c-lightning as a submodule in `external/lightning` - Configure necessary build dependencies

erickcestari · 2025-04-10T19:09:42Z

Draft due to #149

brunoerg · 2025-04-29T18:53:32Z

Closing in favor of #149

erickcestari force-pushed the clightning-rebased branch 2 times, most recently from 73e158c to 17be704 Compare April 6, 2025 16:22

brunoerg reviewed Apr 7, 2025

View reviewed changes

README.md Show resolved Hide resolved

erickcestari force-pushed the clightning-rebased branch from 17be704 to 95ff7e2 Compare April 7, 2025 15:14

brunoerg reviewed Apr 7, 2025

View reviewed changes

modules/clightning/Makefile Show resolved Hide resolved

erickcestari force-pushed the clightning-rebased branch from 95ff7e2 to 9cff17b Compare April 7, 2025 17:12

erickcestari force-pushed the clightning-rebased branch from 9cff17b to 55200a1 Compare April 7, 2025 17:20

erickcestari requested a review from brunoerg April 7, 2025 19:15

erickcestari force-pushed the clightning-rebased branch 2 times, most recently from 467a530 to 0306c99 Compare April 7, 2025 19:17

yuvicc approved these changes Apr 8, 2025

View reviewed changes

erickcestari mentioned this pull request Apr 8, 2025

Enforce minimum witness program length for fallback addresses in BOLT11 parsing ElementsProject/lightning#8219

Merged

4 tasks

erickcestari force-pushed the clightning-rebased branch 2 times, most recently from e6059ab to eb3e102 Compare April 8, 2025 13:47

brunoerg reviewed Apr 9, 2025

View reviewed changes

modules/clightning/clightning_lib.c Show resolved Hide resolved

morehouse reviewed Apr 9, 2025

View reviewed changes

erickcestari force-pushed the clightning-rebased branch from eb3e102 to 1626082 Compare April 9, 2025 16:19

erickcestari changed the title ~~Add c-lightning module for differential fuzzing~~ Add c-lightning module for differential fuzzing - DRAFT Apr 10, 2025

erickcestari marked this pull request as draft April 10, 2025 19:09

brunoerg closed this Apr 29, 2025

Conversation

erickcestari commented Apr 6, 2025

Uh oh!

yuvicc commented Apr 7, 2025

Uh oh!

Uh oh!

brunoerg commented Apr 7, 2025

Uh oh!

brunoerg commented Apr 7, 2025

Uh oh!

erickcestari commented Apr 7, 2025

Uh oh!

Uh oh!

erickcestari commented Apr 7, 2025

Uh oh!

yuvicc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

morehouse Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

erickcestari Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

morehouse Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

erickcestari Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

morehouse Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

morehouse Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

erickcestari commented Apr 10, 2025

Uh oh!

brunoerg commented Apr 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants