Should external C functions check their external object pointer for non-null?

[modelica-trac-importer]

Reported by beutlich on 14 Nov 2016 07:03 UTC
It is neither specified nor clear to me. Should the void pointer of the external object passed to external C functions be checked for non-null. There are three options here:

1. Do not check pointers.

   Advantage: Short and clear code
   Disadvantage: Apllication may crash with null-pointer access violation.

2. Silently accept if the pointer is non-null.

   Advantage: Application no longer crashes with null-pointer access violation.
   Disadvantage: There might be some kind of undefined behavior in case of a null-pointer.

3. Do error handling if pointer is null.

   Advantage: Application no longer crashes with null-pointer access violation and no undefined behavior.
   Disadvantage: Long code and not clear yet, if really necessary.

See also #1436 for a similar issue on character pointer arguments. The question on external object pointers was not yet asked and answered there.

---

Migrated-From: https://trac.modelica.org/Modelica/ticket/2109

[modelica-trac-importer]

Comment by hansolsson on 14 Nov 2016 11:32 UTC
Replying to [ticket:2109 beutlich]:

It is neither specified nor clear to me. Should the void pointer of the external object passed to external C functions be checked for non-null. There are three options here:

I believe we first should answer the obvious question: what if the constructor return a NULL-pointer?

I would say that this is an error, and that an implementation should detect and report this. (And not call the destructor in that case.)

Assuming that we agree on this, then other functions getting a NULL-pointer for an external object indicates an error in the implementation of the Modelica tool; and the rest is then mostly a coding style issue. And 2 or 3 seems preferable; i.e. do nothing or generate error message.

[modelica-trac-importer]

Comment by sjoelund.se on 14 Nov 2016 12:08 UTC
Replying to [comment:1 hansolsson]:

I believe we first should answer the obvious question: what if the constructor return a NULL-pointer?

I would say that this is an error, and that an implementation should detect and report this. (And not call the destructor in that case.)

I would suggest to allow NULL pointers. I use it for when you need a way to call an init function at the start of simulation and I really don't care what the return value is as long as init is called before certain other external function calls. And I don't want to set the pointer to 0xdeadbeef or similar since I want to make it clear this does not point to any valid data.

I would prefer 1. The external code should assume it gets back the pointer from a constructor call, and making sure this is true is the job of the Modelica compiler.

[modelica-trac-importer]

Comment by beutlich on 14 Nov 2016 12:41 UTC
I have seen Modelica tools that pass NULL to external code (by accident of course) that crashed the application. Having 2. or 3. would avoid the crash where only 3. points the user to the problem source.

[modelica-trac-importer]

Comment by hansolsson on 14 Nov 2016 12:41 UTC
Replying to [comment:2 sjoelund.se]:

Replying to [comment:1 hansolsson]:

I believe we first should answer the obvious question: what if the constructor return a NULL-pointer?

I would say that this is an error, and that an implementation should detect and report this. (And not call the destructor in that case.)

I would suggest to allow NULL pointers. I use it for when you need a way to call an init function at the start of simulation and I really don't care what the return value is as long as init is called before certain other external function calls.

Ok. That is also a possibility. And my main point was that this should be clear.

I would prefer 1. The external code should assume it gets back the pointer from a constructor call, and making sure this is true is the job of the Modelica compiler.

I agree that it can assume that the tool does its job. But there are some coding guidelines that require testing for such things - even when they shouldn't happen.

[beutlich]

Two more obersvations:

• @HansOlsson Your reasoning always asumes that the ctor is called after all. But in case a ctor is not called, a null-pointer passed to external functions may crash the application in case of variant 1.
• The ModelicaStandardTables of MSL v3.2.1 and v3.2.2 implement a fourth variant (to the given three variants above).
  4. Silently accept if the pointer is null.

[HansOlsson]

Two more obersvations:
•@HansOlsson Your reasoning always asumes that the ctor is called after all. But in case a ctor is not called, a null-pointer passed to external functions may Crash the application in case of variant 1.

Yes, according to my reasoning calling the external function without calling the ctor is an error in the Modelica tool.

•The ModelicaStandardTables of MSL v3.2.1 and v3.2.2 implement a fourth variant (to the given three variants above).
4. Silently accept if the pointer is null.

I believe that is option 2- and it was just incorrectly written originally.

[beutlich]

I believe that is option 2- and it was just incorrectly written originally.

Right. I was confused. If the pointer is null, ModelicaStandardTables_CombiTable1D_getValue returns a default value of zero (without any message). This is variant 2 and that leads to unexpected behaviour.

@HansOlsson If you and @sjoelund prefer variant 1, we should add the non-null attributes then. Right?

[HansOlsson]

@HansOlsson If you prefer variant 1, we should add the non-null attributes then. Right?

Likely, but I don't prefer option 1. The reason is that we don't like the possibility of such crashes.

What happens if we add non-null attribute and use option 2 - i.e. silently accept something that never should happen.

[beutlich]

What happens if we add non-null attribute and use option 2 - i.e. silently accept something that never should happen.

This also would be my preferred solution. In VS SAL notation:

• _Ret_notnull_ should be added to the ctor
• _In_ should be added to the external object void* parameter for all other functions incl. dtor.

[HansOlsson]

This also would be my preferred solution. In VS SAL notation:
•Ret_notnull should be added to the ctor
•In should be added to the external object void* parameter for all other functions incl. dtor.

Seems good. An implicit part of this is that the ctor shall use ModelicaError (etc) to report error - and not return a null-pointer.