Correctly handle memory and functional behavior of TLVs about libotr-ng

[juniorz]

In an attempt to summarize the mentioned discussions, I believe this is about: Double check if "auto-generated" TLVs (padding, and maybe the SMP-related - see #83) can work together with user's TLVs.

I believe the concern here is memory management. Double check how the API can be used to send a message with no user's TLVs (using NULL should be enough), and review append_padding_tlv thinking about memory ownership.

[olabini]

OK, I'll take a look at the points and places you mentioned. But as you say, some of this work is also part of the other two issues. I'll see what I can do, and then declare this issue finished. =)

[olabini]

Well, the function is set_tlv_type. So, if we don't assign anything, the TLV will end up having type PADDING - which is not correct. So the alternative is that set_tlv_type returns a boolean or error type. It's generally not very common for "setters" to have a result like that, but sure, that's possible.

However, the real problem comes later. set_tlv_type is called from parse_tlv, which returns the newly created TLV. I assume your proposal is that we return an error or NULL from that function - if we the current code doesn't have a specific error type, instead, NULL is returned to mark problems with allocation, so NULL actually is an error. But getting an unknown type is not an error, so this function has to change to return both a potential TLV, and an error, and any consumers of that dealing with those alternatives.

Does that sound like what you think should be done?

[olabini]

@claucece - could you please expand on "Some TLVs that don't have the 'value' type are been putted like so, for example." - I'm not sure I understand what you mean here.

[olabini]

OK, the only thing left on this one is the final point needing clarification from @claucece - and then a once over of all the TLV usage in the code base. But it looks pretty good right now.

[claucece]

@olabini This is part of a very big question. First the SMP tlvs are not really tested from an API level. See this test: https://github.com/otrv4/libotr-ng/blob/master/src/test/test_smp.c#L25 that returns prior to be executed. On tests that should test the API (and functions from there, for example, the ones used for a state machine), we are using tlv unit functions. See: https://github.com/otrv4/libotr-ng/blob/master/src/test/test_api.c#L171. Here otrng_tlv_new is being used, when something like otrng_smp_start or similar should be used. The way otrng_tlv_new does not even represent a real TLV: the value is completely arbitrary. Should be used something like this: https://github.com/otrv4/libotr-ng/blob/master/src/test/test_api.c#L1230 (not ideal, but that actually tests the API, not the unit TLV functions). This perhaps is part of issue #101 or #83

On a side note, do you think we still need a OTRNG_TLV_NONE?

[olabini]

About OTRNG_TLV_NONE - the only place it can happen is when we receive a TLV type that we don't recognize. Since that is something that can happen, and we're using an enum type for the TLV type, I do think we need to keep it.

[olabini]

OK, I have now fixed up some of the tests too. I think most of the stuff around TLVs have been fixed. The problems with the API tests are wider than can be covered in this issue, but I've sorted out the issues @claucece pointed out in the above comment.

[claucece]

Great! Ok, I'll add some further test checking to #101 .

@olabini I vote for removing OTRNG_TLV_NONE.

[olabini]

OK. So about OTRNG_TLV_NONE, what should we do when we receive an unknown TLV?

[claucece]

So about OTRNG_TLV_NONE, what should we do when we receive an unknown TLV?

For me, error (return NULL, or bool or whatever is been used). Right now, it seems like we are creating a tlv with type OTRNG_TLV_NONE, which shouldn't even exist.

[olabini]

Yeah. We have one or two other places that ignore that TLV, just as we ignore Padding TLVs.
But remember, receiving an unknown TLV is not an error. It's completely allowed by the spec, and we should allow it and treat it by ignoring it.

[juniorz]

I second what @olabini said, they are still "syntactically" valid TLVs that OTRv4 does know what to do. Maybe rename `OTRNG_TLV_NONE` to `OTRNG_TLV_UNKNOWN` or `OTRNG_TLV_UNSUPPORTED`, if the `NONE` is the source of confusion.

[claucece]

Mmm...

It's completely allowed by the spec, and we should allow it and treat it by ignoring it.

Yeah. That is why for me, when an unknown type comes, we should not assign anything to it (not a type) and just ignore it (by returning or similar).

Maybe rename OTRNG_TLV_NONE to OTRNG_TLV_UNKNOWN or
OTRNG_TLV_UNSUPPORTED, if the NONE is the source of confusion.

No. It is more about assigning something to something that shouldn't, I guess. ;)

[claucece]

@olabini Then I guess we should keep OTRNG_TLV_NONE. Thanks.