fix for double freeing of memory.

[ihsinme]

my analysis shows that it is possible to achieve double freeing of memory. to eliminate it, I propose a simple fix.

[valeriuo]

Well, your fix broke the build, since e is undefined.
Any insights into your analysis would be welcome.

[daviesrob]

I think the problem is that the loop that constructs @SQ lines from a .fai file here doesn't set sn = NULL; at the end (compare with the earlier loop). The data pointed to by sn will either become a hash table key, or will be freed because it's a duplicate. Either way, sn should be set to NULL to prevent either a double free, or a dangling pointer in the hash table.

[ihsinme]

I think this is a more elegant fix. assign zero after release. If you agree, I will change PR in this direction?

[daviesrob]

On looking a bit deeper, this problem is actually quite complicated. I think a few changes may be needed to get it to work correctly in all the places where it tries to clean up after an error. In particular, sn needs to be set to NULL immediately after the calls to free(sn), or after it's known to be in the d hash table. Then the addition to the long_refs hash should be changed to:

k2 = kh_put(s2i, long_refs, kh_key(d, k), &absent);

as sn will no longer be set. That should ensure the string is either pointed to by sn or a d hash key, but not both.

This change will be needed in both places where the hash table inserts occur.

[ihsinme]

I don't understand how your product works so deeply.
so tell me if it is worth making a similar change in the first cycle.

[valeriuo]

Yes, the multiple exit points out of that second loop and the fact that the pointers to the reference names are shared by d, long_refs and target_name make the memory management tricky. Even with this patch, if the loop exists at the first attempt to insert into d, the first sn will leak.

[ihsinme]

please note the sn = NULL; on line 1861.
has it become redundant?

[daviesrob]

I'm beginning to think that the best solution would be to pull the hash table filling parts into a new static inline function. It would both make the error handling easier and remove some duplication. If it took a const char * and length, it could handle allocating the copy of the string which would keep the error recovery part much more local. The bit that looks for the name in the @SQ line would need altering to store the location of the name in the line it's just read along with the length.

[valeriuo]

please note the sn = NULL; on line 1861.
has it become redundant?

Yes, there was no need to set the sn to NULL everywhere. So I reverted your latest commit.
I also added some other changes:

• First of all, there should be no goto error in the second loop, as this prevents freeing line, which happens after the loop ends.
• Second, all loop breaks that resulted from an error now set the variable e, which triggers the error handling mode, to prevent the successful return of a header with malformed data.

[daviesrob]

I think you need some changes to the first loop as well, as it can still jump to error with the pointer in both sn and the hash key (e.g. if long_refs can't be allocated or if adding to it fails).