From 76813d6f90b384a5176110fdae8bf1a1a946a94b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 25 Aug 2020 12:54:13 +0930 Subject: [PATCH] tools/generate-wire.py: fix varsize assignment. Code like this is suspicious: subtype_varsize_struct->field_0 = *fromwire_test_features(subtype_varsize_struct, cursor, plen); In fact, it is a memory leak since we copy and don't free the fromwire result. Really, field_0 should be a pointer. We don't hit this case (yet!) in spec-generated code, but I did for bolt13. Here's the difference in gen_test output: ```patch diff -ur /tmp/before/test/gen_test.c /tmp/after/test/gen_test.c --- /tmp/before/test/gen_test.c 2020-05-07 16:23:31.651611235 +0930 +++ /tmp/after/test/gen_test.c 2020-05-07 16:20:54.232574482 +0930 @@ -214,12 +214,12 @@ static void towire_subtype_varsize_struct(u8 **p, const struct subtype_varsize_struct *subtype_varsize_struct) { - towire_test_features(p, &subtype_varsize_struct->field_0); + towire_test_features(p, subtype_varsize_struct->field_0); } static void fromwire_subtype_varsize_struct(const u8 **cursor, size_t *plen, struct subtype_varsize_struct *subtype_varsize_struct) { - subtype_varsize_struct->field_0 = *fromwire_test_features(subtype_varsize_struct, cursor, plen); + subtype_varsize_struct->field_0 = fromwire_test_features(subtype_varsize_struct, cursor, plen); } /* SUBTYPE: SUBTYPE_VAR_LEN */ @@ -373,7 +373,7 @@ ptr = tal_arr(ctx, u8, 0); - towire_test_features(&ptr, &r->tlv3->features); + towire_test_features(&ptr, r->tlv3->features); towire_amount_msat(&ptr, r->tlv3->amount_msat_1); @@ -385,7 +385,7 @@ struct tlv_test_n1 *r = vrecord; r->tlv3 = tal(r, struct tlv_test_n1_tlv3); - r->tlv3->features = *fromwire_test_features(r->tlv3, cursor, plen); + r->tlv3->features = fromwire_test_features(r->tlv3, cursor, plen); r->tlv3->amount_msat_1 = fromwire_amount_msat(cursor, plen); r->tlv3->amount_msat_2 = fromwire_amount_msat(cursor, plen); } @@ -824,11 +824,11 @@ towire_test_short_id(&ptr, &r->tlv3->subtype); - towire_subtype_var_len(&ptr, &r->tlv3->varlen_subtype); + towire_subtype_var_len(&ptr, r->tlv3->varlen_subtype); - towire_subtype_var_assign(&ptr, &r->tlv3->varlen_assigned); + towire_subtype_var_assign(&ptr, r->tlv3->varlen_assigned); - towire_subtype_varlen_varsize(&ptr, &r->tlv3->test_sbt_varlen_varsize); + towire_subtype_varlen_varsize(&ptr, r->tlv3->test_sbt_varlen_varsize); for (size_t i = 0; i < 2; i++) towire_u32(&ptr, r->tlv3->arr_assign[i]); @@ -868,9 +868,9 @@ r->tlv3 = tal(r, struct tlv_test_n3_tlv3); fromwire_test_short_id(cursor, plen, &r->tlv3->subtype); - r->tlv3->varlen_subtype = *fromwire_subtype_var_len(r->tlv3, cursor, plen); - r->tlv3->varlen_assigned = *fromwire_subtype_var_assign(r->tlv3, cursor, plen); - r->tlv3->test_sbt_varlen_varsize = *fromwire_subtype_varlen_varsize(r->tlv3, cursor, plen); + r->tlv3->varlen_subtype = fromwire_subtype_var_len(r->tlv3, cursor, plen); + r->tlv3->varlen_assigned = fromwire_subtype_var_assign(r->tlv3, cursor, plen); + r->tlv3->test_sbt_varlen_varsize = fromwire_subtype_varlen_varsize(r->tlv3, cursor, plen); for (size_t i = 0; i < 2; i++) { u32 tmp; tmp = fromwire_u32(cursor, plen); diff -ur /tmp/before/test/gen_test.h /tmp/after/test/gen_test.h --- /tmp/before/test/gen_test.h 2020-05-07 16:23:30.399617108 +0930 +++ /tmp/after/test/gen_test.h 2020-05-07 16:20:52.912584680 +0930 @@ -45,7 +45,7 @@ struct test_short_id field_1; }; struct subtype_varsize_struct { - struct test_features field_0; + struct test_features *field_0; }; struct subtype_var_len { struct test_short_id *field_2; @@ -60,15 +60,15 @@ struct test_short_id field3[2]; }; struct tlv_test_n1_tlv3 { - struct test_features features; + struct test_features *features; struct amount_msat amount_msat_1; struct amount_msat amount_msat_2; }; struct tlv_test_n3_tlv3 { struct test_short_id subtype; - struct subtype_var_len varlen_subtype; - struct subtype_var_assign varlen_assigned; - struct subtype_varlen_varsize test_sbt_varlen_varsize; + struct subtype_var_len *varlen_subtype; + struct subtype_var_assign *varlen_assigned; + struct subtype_varlen_varsize *test_sbt_varlen_varsize; /* array assigtest_nable */ u32 arr_assign[2]; /* array structs */ Binary files /tmp/before/test/gen_test.o and /tmp/after/test/gen_test.o differ Binary files /tmp/before/test/run-test-wire and /tmp/after/test/run-test-wire differ Binary files /tmp/before/test/run-test-wire.o and /tmp/after/test/run-test-wire.o differ ``` Signed-off-by: Rusty Russell --- tools/gen/header_template | 2 +- tools/gen/impl_template | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/gen/header_template b/tools/gen/header_template index ea03b9bee..2c0caad52 100644 --- a/tools/gen/header_template +++ b/tools/gen/header_template @@ -49,7 +49,7 @@ struct ${struct.struct_name()} { % endif % if f.is_varlen() and f.type_obj.is_varsize(): ${f.type_obj.type_name()} **${f.name}; - % elif f.is_varlen(): + % elif f.is_varlen() or f.type_obj.is_varsize(): ${f.type_obj.type_name()} *${f.name}; % elif f.is_array(): ${f.type_obj.type_name()} ${f.name}[${f.count}]; diff --git a/tools/gen/impl_template b/tools/gen/impl_template index 341175bee..c255506a8 100644 --- a/tools/gen/impl_template +++ b/tools/gen/impl_template @@ -64,7 +64,7 @@ towire_${type_obj.name}(${ptr}, ${f.name}); % elif is_single_ptr: towire_${type_obj.name}(${ptr}, ${'*' if type_obj.is_assignable() else ''}${fieldname}); % else: -towire_${type_obj.name}(${ptr}, ${'' if type_obj.is_assignable() else '&'}${fieldname}); +towire_${type_obj.name}(${ptr}, ${'' if type_obj.is_assignable() or type_obj.is_varsize() else '&'}${fieldname}); % endif ## Subtype and TLV-msg fromwire @@ -110,7 +110,7 @@ ${fieldname} = ${f.size('*plen')} ? tal_arr(${ctx}, ${typename}, 0) : NULL; % if f.type_obj.is_assignable(): ${ f.name if f.len_field_of else fieldname} = fromwire_${type_}(cursor, plen); % elif f.type_obj.is_varsize(): -${fieldname} = *fromwire_${type_}(${ctx}, cursor, plen); +${fieldname} = fromwire_${type_}(${ctx}, cursor, plen); % else: fromwire_${type_}(cursor, plen, &${fieldname}); % endif