invalid alloc size 4 at ytnef.c : 1184

[mjog]

Hi there,

Geary is using ytnef for TNEF support and its CI tests have started failing using ytnef master over the last few days with the following:

4853 ERROR: invalid alloc size 4 at ytnef.c : 1184, suspected corruption (exceeded 0 bytes)

Looks related to https://github.com/Yeraze/ytnef/pull/79?

[mjog]

Example logs from CI test failure are here: https://gitlab.gnome.org/GNOME/geary/-/jobs/697329

The failing test is loading TNEF data from this file: https://gitlab.gnome.org/GNOME/geary/-/blob/mainline/test/data/basic-multipart-tnef.eml

[Yeraze]

interesting.. It seems to be working fine for me. Are you using the latest release or mainline?

[mjog]

The Flatpak CI build pulls ytnef git master and builds that, the other CI pipelines use whatever Ubuntu and Fedora have packaged - currently the latest release. It's only the Flatpak CI build that has this issue, and it's only started happening in the last few days.

IIRC, the Flatpak builds with some stack hardening features in place, maybe that's why you're not seeing it locally? Does valgrind's memcheck report any issues?

[Yeraze]

I'm not familiar enough with the Geary codebase to know, but in the latest master there was a change to the Initialization Struct to set the maximum attachment size. If you're calling TNEFInitialize from the proper library, it should be initializing to 50Meg, but if you're using an older version of the header without that 1-line change, valgrind's 0-memory initialization may be defaulting it to 0. (which would cause the error you reported).

[ohwgiles]

With zero-initialized structs, calling TNEFInitialize wasn't necessary until that change. Of course it should have been done anyway, that looks like my oversight. Hopefully this should fix it.

[Yeraze]

In addition, you can directly set TNEF->attachmentSize to some number to define what the limit is (if you have UI or other way to setting it). It's an integer of the maximum size in MB (should Default to 50).

[mjog]

Looks like even with the TNEFInitialize call in place it's still a problem: https://gitlab.gnome.org/ogtifs/geary/-/jobs/698358

The default attachments size is probably fine given contemporary RAM configs and email attachment size limits :)

[Yeraze]

Have you tried building this locally? I'm on a mac, I don't think I can build this here... But from looking through the console log of that build, I see right after it retrieved the ytnef GIT repo, it then says it had a cache-hit and didn't build it. Could it be pulling an older ytnef from the filesystem?

The sample file you're failing on is already in the Ytnef automated tests and works fine.. so I'm not sure what's going on.

[mjog]

Hmm, so what's the semantics of TNEFParseMemory (and hence TNEFParse) wrt the TNEF param? It's a caller-initialised, pass-by reference (as in pass-by-pointer) value, right? Those functions expect it to be initialised by the caller, take a pointer to it as an arg, then modify the structure in-place via the pointer?

If so I think the vala bindings may need an update as well, because as-is the out modifier on that method causes vala to assume the value is callee-initialised. This wasn't a problem under the old init regime, but clearly would be with the requirement to use TNEFInitialize.

[mjog]

One other question, it looks like we should be freeing TNEFStruct with a call to TNEFFree, I assume that's mandatory?

[Yeraze]

Well, TNEFInitialize and TNEFFree are not "mandatory" per se, they're just convenience to initialize and free all the memory with a single call. Simply destroying the struct may leave leaked memory allocated along the linked-lists .. You can easily refer to the code and see what they do and recreate to your convenience.

[ohwgiles]

It's a caller-initialised, pass-by reference (as in pass-by-pointer) value, right? Those functions expect it to be initialised by the caller, take a pointer to it as an arg, then modify the structure in-place via the pointer?

This is my understanding too.

If so I think the vala bindings may need an update as well, because as-is the out modifier on that method causes vala to assume the value is callee-initialised. This wasn't a problem under the old init regime, but clearly would be with the requirement to use TNEFInitialize.

TNEFStruct is not allocated by libytnef at all, it must be allocated by the library user. However, immediately following allocation it should be initialized with TNEFInitialize, which expects a pointer. So it is callee-initialised (just not callee-allocated). I don't know Vala, but it seems like it would be nice if there were a way to express this in the binding, so the user of the binding didn't have to know this detail.

it looks like we should be freeing TNEFStruct with a call to TNEFFree

I thought setting destroy_function="TNEFFree" would do that but perhaps it doesn't?

[mjog]

Yup, so we can make sure TNEFInitialize is always called by adding a constructor for the struct in the VAPI that is bound to it, and then ensure we actually call the constructor in the vala code:

var tn = Ytnef.TNEFStruct();

But then the use of the out modifier in the VAPI is when calling TNEFParseMemory (and probably also MAPIFindProperty) is bad because that indicates the struct is constructed in the function and passed back out to the caller, which is the opposite of what that function is actually doing. Since under vala structs are pass-by-reference, using the ref modifier would be appropriate instead.

[mjog]

@ohwgiles NB, my bad on the TNEFFree thing, missed that in the VAPI.

Lets take this conversation over to the Geary MR though - I no longer think there's anything to be fixed code-wise with ytnef here.

@Yeraze:

Well, TNEFInitialize and TNEFFree are not "mandatory" per se, they're just convenience to initialize and free all the memory with a single call.

Well, they're only optional as long as TNEFStruct doesn't grow additional members with non-default initialisation or de-initialisation assumptions or change same for existing members (as happened here), or if one is happy with memory leaks. So users of the library we have the choice of either calling these methods, or have our programs crash at random points in the future, which isn't much of a choice. :)

Anyway, it's entirely reasonable to be changing these assumptions in a release that bumps the major version number, but some documentation that is was a breaking change would have been welcome. The Changelog entry doesn't suggest that this change requires action to be taken on the part of library users ("Make sure you are calling TNEFInitialize or initialise this new thing manually yourself, else your program will start crashing").