Repository: EmbeddedRPC/erpc (language: python)

Issue:
Fixed union free function by always adding all union type cases.
See #69.

Fixed the issue by adding all cases to the switch statement even if there are empty.

This fixes the issue because if now a free statement is needed in the default case it is only called if it's none of the other cases.

Memory violation in free_*_struct function when using unions
Hello!

When using an `union` within a struct the free-function does not handle all switch-cases but only those that needs special sub-free-calls like `list<>` etc. Unfortunately this free calls is then always performed regardless of which union type was used at that moment.

Example eRPC IDL:
```
struct TestStruct
{
	int32 handle_count // uint32 handle_count causes error in eRPC code
    union(handle_count)
	{
	case 0:
		uint16 start_handle
		uint16 end_handle
	case 1:
		uint16 handle
		uint16 offset
	default:
		list<uint16> handles
	} range
}
```

The free call looks like this:
```
static void free_TestStruct_struct(TestStruct * data)
{
    switch (data->handle_count)
    {
        default:
        {
            free_list_uint16_1_t_struct(&data->range.handles);
            break;
        }
    }
}
```

This will lead to memory violation if the list was not in use.


Discussion / hints:
> 
> 
> See #69.
> 
> Fixed the issue by adding all cases to the switch statement even if there are empty.
> 
> This fixes the issue because if now a free statement is needed in the default case it is only called if it's none of the other cases.


Hi @burgerkingorama, 
thank you for your interest. For code size /readability optimization would be better to revert your commit changes and add new for loop for cases which don't need to free data. 
I think your output will be:
`
static void free_TestStruct_struct(TestStruct * data)
{
    switch (data->handle_count)
    {
	case 0:
		break;
	case 1:
		break;
       ....
       case n:
             break;
        default:
        {
            free_list_uint16_1_t_struct(&data->range.handles);
            break;
        }
    }
}`

Suggested:
`static void free_TestStruct_struct(TestStruct * data)
{
    switch (data->handle_count)
    {
	case 0:
	case 1:
        ....
        case n:
		break;
        default:
        {
            free_list_uint16_1_t_struct(&data->range.handles);
            break;
        }
    }
}`

Fix patch:
diff --git a/erpcgen/src/CGenerator.cpp b/erpcgen/src/CGenerator.cpp
index 6b732e45..91e72d64 100644
--- a/erpcgen/src/CGenerator.cpp
+++ b/erpcgen/src/CGenerator.cpp
@@ -2682,6 +2682,7 @@ data_map CGenerator::getEncodeDecodeCall(const string &name, Group *group, DataT
 
                 data_list unionCases;
                 data_list unionCasesToFree;
+                bool needCaseEmptyFreeingCall = false;
                 // call free function for this union, default not call any free function
                 templateData["freeingCall"] = make_template("", &params);
                 for (auto unionCase : unionType->getCases())
@@ -2731,6 +2732,7 @@ data_map CGenerator::getEncodeDecodeCall(const string &name, Group *group, DataT
                             {
                                 // current member don't need free memory
                                 memberData["isNeedFreeingCall"] = false;
+                                needCaseEmptyFreeingCall = true;
                             }
                             caseMembers.push_back(memberData);
                             if (casesNeedTempVariable)
@@ -2743,6 +2745,7 @@ data_map CGenerator::getEncodeDecodeCall(const string &name, Group *group, DataT
                     unionCases.push_back(caseData);
                 }
                 templateData["cases"] = unionCases;
+                templateData["needCaseEmptyFreeingCall"] = needCaseEmptyFreeingCall;
                 templateData["encode"] = m_templateData["encodeUnionType"];
                 templateData["decode"] = m_templateData["decodeUnionType"];
             }
diff --git a/erpcgen/src/templates/c_coders.template b/erpcgen/src/templates/c_coders.template
index e9d72106..357f9a14 100644
--- a/erpcgen/src/templates/c_coders.template
+++ b/erpcgen/src/templates/c_coders.template
@@ -168,11 +168,7 @@ codec->startReadUnion({% if info.castDiscriminator %}{% if info.dataLiteral != "
 switch ({% if info.dataLiteral != "" %}{$info.dataLiteral}{% else %}*{% endif %}{$info.discriminatorName})
 {
 {% for case in info.cases %}
-{%  if case.name == "default" %}
-    default:
-{%  else %}
-    case {% if case.name != "" %}{$case.name}{% else %}{$case.value}{%  endif %}:
-{%  endif -- default or case %}
+    {% if case.name != "default" %}case {% endif %}{% if case.name != "" %}{$case.name}{% else %}{$case.value}{%  endif %}:
     {
 {%  for member in case.members %}
 {% if source == "server" %}
@@ -295,11 +291,7 @@ codec->startWriteUnion({% if info.castDiscriminator %}static_cast<int32_t>({% en
 switch ({$info.dataLiteral}{$info.discriminatorName})
 {
 {% for case in info.cases %}
-{%  if case.name == "default" %}
-    default:
-{%  else %}
-    case {% if case.name != "" %}{$case.name}{% else %}{$case.value}{% endif %}:
-{%  endif -- default or case %}
+    {% if case.name != "default" %}case {% endif %}{% if case.name != "" %}{$case.name}{% else %}{$case.value}{% endif %}:
     {
 {%  for member in case.members %}
 {$> addIndent("        ", member.coderCall.encode(member.coderCall))}
diff --git a/erpcgen/src/templates/c_common_functions.template b/erpcgen/src/templates/c_common_functions.template
index aae030cc..37d2e426 100644
--- a/erpcgen/src/templates/c_common_functions.template
+++ b/erpcgen/src/templates/c_common_functions.template
@@ -395,13 +395,17 @@ for (uint32_t {$info.forLoopCount} = 0; {$info.forLoopCount} < {$info.size}; ++{
 {% def freeUnion(info) %}
 switch ({$info.dataLiteral}{$info.discriminatorName})
 {
+{%if info.needCaseEmptyFreeingCall %}{% for case in info.cases %}
+{%  if case.needCaseFreeingCall == false %}
+    {%   if case.name != "default" %}case {% endif%}{% if case.name != "" %}{$case.name}{% else %}{$case.value}{% endif %}:
+{%  endif %}
+{% endfor -- cases %}
+    {
+        break;
+    }{%    endif -- needCaseEmptyFreeingCall %}
 {% for case in info.cases %}
 {%  if case.needCaseFreeingCall == true %}
-{%   if case.name == "default" %}
-    default:
-{%   else %}
-    case {% if case.name != "" %}{$case.name}{% else %}{$case.value}{% endif %}:
-{%   endif -- default or case %}
+    {%   if case.name != "default" %}case {% endif%}{% if case.name != "" %}{$case.name}{% else %}{$case.value}{% endif %}:
     {
 {%   for member in case.members %}
 {%    if member.isNeedFreeingCall %}


Test patch:
diff --git a/erpcgen/test/test_union_c.yml b/erpcgen/test/test_union_c.yml
index 5a2b785c..af4ca69c 100644
--- a/erpcgen/test/test_union_c.yml
+++ b/erpcgen/test/test_union_c.yml
@@ -48,8 +48,6 @@ idl: |
       binary rawString
   }
 
-  //  const int32 returnVal = 10;
-
   union unionType
   {
     case apple:
@@ -203,6 +201,14 @@ test_server.cpp:
   - "{"
   - switch (discriminator)
   - "{"
+  - "case banana:"
+  - not: "break;"
+  - "case papaya:"
+  - "case returnVal:"
+  - "default:"
+  - "{"
+  - "break;"
+  - "}"
   - "case apple:"
   - "{"
   - free_foobar_struct(&data->myFoobar);
@@ -277,4 +283,4 @@ test_server.cpp:
 
 #test_server.cpp:
 #  - static int32_t read_unionType_union(erpc::Codec * codec, int32_t discriminator, unionType * data);
-#  - static int32_t read_unionType_union(erpc::Codec * codec, int32_t discriminator, unionType * data)
\ No newline at end of file
+#  - static int32_t read_unionType_union(erpc::Codec * codec, int32_t discriminator, unionType * data)