segfault on reusing a structure after ASN_STRUCT_FREE_CONTENTS_ONLY().

[dbaarda]

I'm not sure if this is a bug or working as intended. I didn't encounter this problem with v0.9.24, and am seeing it now with 0.9.27, but maybe I just got lucky before. I also switched from native-types to wide-types which may have contributed.

When trying to reuse a message structure after using and then "emptying" it, setting attributes using things like OCTET_STRING_fromBuf() can segfault. An example LDAP code fragment looks like this;

msg = calloc(1,sizeof(LDAPMessage_t))
msg->messageID = msgid;
res->protocolOp.present = LDAPMessage__protocolOp_PR_searchResEntry;
SearchResultEntry_t *searchResEntry = &res->protocolOp.choice.searchResEntry;
/* Populate the msg searchResEntry attributes here... */
...
/* Now empty the msg and reuse it for a searchResDone response. */
ASN_STRUCT_FREE_CONTENTS_ONLY(asn_DEF_LDAPMessage, msg);
msg->protocolOp.present = LDAPMessage__protocolOp_PR_searchResDone;
SearchResultDone_t *searchResDone = &res->protocolOp.choice.searchResDone;
/* And now this will segfault... */
OCTET_STRING_fromString(&searchResDone->matchedDN, basedn);

The problem is that OCTET_STRING_fromString() tries to free the existing matchedDN attribute before alloc/set the new one. However, the existing matchedDN attribute contains a garbage pointer left over from the previous searchResEntry message.

I realize this is pretty normal C behavior; freeing things doesn't re-initialize/zero the pointers, and it's up to the caller to do that. However, there appears to be some effort in things like OCTET_STRING_free() to leave the free'd structure in a valid initialized state, doing things like zeroing the pointer after free. Digging around other *_free() functions it looks like most others don't do this.

I think the right thing here is to adhere to C conventions and have ASN_STRUCT_FREE_CONTENTS_ONLY() offer no guarantees that the structure is left in a valid initialized/zeroed state. In that case I think *_free() functions should never expend effort on things like zeroing pointers after free(), because that just masks that this cannot be relied on.

[mouse07410]

I thought about changing OCTET_STRING_free() to stop zeroing the pointer to the allocated string buffer, or about changing all the constr_<whatever>_free() to make them all zero the buffer pointer if the member happens to be an OCTET STRING... But the logic says that if the member of a constructed type is an OCTET STRING, then OCTET_STRING_free() would be invoked - and it should zero the relevant pointer, thus preventing the reported problem from happening.

@velichkov what do you think about this?

[dbaarda]

The problem bites you when the OCTECT_STRING is inside a "choice" union. It doesn't matter if the pointer was zero'ed in one of the "choice" message structures, if you then re-use the message for a different message type with a different structure, it may have OCTECT_STRING elements with a different alignment in the union. This means the new message's OCTECT_STRING could have a random garbage pointer from other parts of the previous message structure in the union.

Note I didn't hit this problem before 0.9.27 and I switched to native types, but I suspect I just got lucky with the alignment of the elements in the different message types using non-native types.

It's not a big deal once you know that ASN_STRUCT_FREE_CONTENTS_ONLY() doesn't guarantee that the message is initialized into a sane state and you need to zero it yourself. The main niggling issue is it ALMOST initializes it into a sane state, wasting a few cycles doing so, and giving you a false impression that it does.

IMHO it should either free and initialize it properly (which would maybe involve the CHOICE_free() zeroing the whole union), or don't initialize it at all. I lean towards the latter choice since it's consistent with the behavior of free(), but you probably want to make it clear either way what the behavior is.

[mouse07410]

The problem bites you when the OCTET_STRING is inside a CHOICE union.

I see. Does it apply to SET as well?

IMHO it should either free and initialize it properly (which would maybe involve the CHOICE_free() zeroing the whole union), or don't initialize it at all.

I lean towards the first choice (no pun intended :). I wonder if the whole thing should be zeroed in case of SET as well?

@velichkov, your input is welcome here! @brchiu what's your opinion? If CHOICE_free() would zero the entire thing, would there be any negative/undesirable consequences?

[brchiu]

A small problem is these xxxx_free() function only knows the pointer but not the structure size, for example LDAPMessage_t in this case, if we want to zero the entire structure, we might have to tweak a little to include the sizeof(LDAPMessage_t) information in asn_DEF_LDAPMessage.

My bad, asn_SPC_LDAPMessage_specs_1 already has the size information. But just wonder the following quick fix is useful or not.

@dbaarda In skeleton\asn_internal.h, if we change :

#define	FREEMEM(ptr)		do { free(ptr); ptr = (typeof(ptr))NULL; } while(0)

Could this be helpful ?

[dbaarda]

The problem is it's not enough to zero out the pointers, you need to zero out everything, including native integers and doubles since a different message type will overlay a different struct in the union over that value, which might end up putting a pointer where that native integer or double was before.

If you are really going to do this, then every *_free() for "leaf" elements must zero out it's entire contents if contents_only is true (if contents_only is false, the whole thing is going to be free'd and thus doesn't need to be wiped). Container elements like CHOICE and SET can rely on their contained elements zeroing their own contents, and just need to zero any extra supporting struct fields like count's or choice values.

[brchiu]

@dbaarda Got it.

Again, I found the problem that not knowing the size of previous allocated memory indeed exists for *_free(), unlike LDAPMessage, if these types 'inherits' from other types and specifics field is null in generated file.

Taking DelRequest of generated LDAP3 files as an example, it inherits from LDAPDN. LDAPDN inherits from LDAPString which inherits from OCTET_STRING. Since OCTET_STRING is a base type (at least in certain sense) having non-null specifics field indicates its size is sizeof(OCTET_STRING_t).

So the task is to have a suitable way, when we want to zero out memory allocated for DelRequest, we know we have to zero out sizeof(OCTET_STRING_t) bytes.

Good news is : PR #93 has done this base type finding task.
Bad news is : it has not been accepted in vlm's repository yet.

However, since @mouse07410's repository has included this #93, through merging #143 indirectly. Perhaps I can implement the idea there.

Please stay tuned.

After a quick look into skeleton/*.c with *_free(), things are not that simple.
Let me re-think it in detail. Sorry.

[mouse07410]

@brchiu I agree - it isn't that simple.

I'm considering perhaps just documenting that if

a. the PDU includes CHOICE or SET element(s), and
b. the re-use may involve different "sub-objects" (like what @dbaarda described),

then using ASN_STRUCT_FREE_CONTENTS_ONLY() is inappropriate and would likely lead to errors/crashes.

[brchiu]

hi, Both,

Based on @mouse07410's repository master branch, I partially implement a fix to tackle this issue in my own repository zero_out_structure_when_free_content_only.

The reason I said 'partially' because only SET_free(), SET_OF_free(), SEQUENCE_free(), CHOICE_free() and OCTET_STRING_free() are modified to zero out allocated memory.

There are BOOLEAN_free(), NativeInteger_free(), NativeReal_free() and the ASN__PRIMITIVE_TYPE_free() have not be handled due to lack information like struct_size field of *_specific_t structure.

Review and advice are welcomed.

[dbaarda]

I think if anything that has a union zero's out the space used by that union, then the problem will be mostly fixed. I think since you've covered the "container" types, I think you have it covered.

[mouse07410]

I think if anything that has a union zero's out the space used by that union, then the problem will be mostly fixed. I think since you've covered the "container" types, I think you have it covered.

I agree in principal, but I thought that only CHOICE and SET (and maybe SET OF) would qualify as "union" types?

Also, let's continue this discussion here: #146 mouse07410#12

[mouse07410]

In the meanwhile, hopefully fixed in the mouse-master.

[vlm]

@mouse07410 or @brchiu, would you please post a single comment summary of the arguments and a suggestion in this patch? So far I am leaning towards fixing the _FREE/_free semantics to be non-zeroing, and creating ASN_STRUCTURE_CLEAN macro to explicitly deal with application level corner-cases. The motivating example seems to be just a wrong use of abstraction, so it does not look convincing.

[brchiu]

@vlm, I don't mind to revoke this pr if there is better way to solve (or prevent users' wrong usage) this issue. You can just do what you think is the best on your solely discretion.

Thank you for your great effort.

[mouse07410]

@vlm I think that since the patch does fix the problem without imposing undue restriction on what a reasonable user would expect, and is fairly simple/small - it should be in.

[vlm]

I was reluctant to merge mouse07410#12, because it introduces recursive memory resetting behavior when contents_only option is present. Consider the hierarchical structures. Freeing them would memset() each of the inner structures, then memset() the outer structure, and so on. This essentially resets each byte of the structure as many times as the number of layers in the hierarchy. This is wasteful and unnecessary.

Therefore I introduced ASN_STRUCT_RESET() macro which does clear the contents, and discouraged using ASN_STRUCT_FREE_CONTENTS_ONLY() in the application code.

8d99d7b