#26 Dangerous error handling

[tbeu]

The current error handling with calling Mat_VarFree immediately after Mat_Error or Mat_Critical is dangerous. I noticed a crash of the matdump tool if a severe error occurs during reading a MAT file. The matvar is then freed first in the matio library and a second time in matdump.c (line 907 or 913). The simple reason for the crash is that the struct members of matvar are not nullified in Mat_VarFree after freeing. Thus I propose to also nullify freed variables.

[tbeu]

If you are interested I'll see if I can provide a patch.

[cch]

Thomas,
A patch would be good and I'm ok with making all the variables 0 (maybe just a memset call would be the easiest. I'm not sure there is a system where NULL is not all zeros).

However, If an error occurs in "Mat_VarReadNext" or "Mat_VarReadNextInfo", NULL should be returned so that mat dump does not call "Mat_VarFree". Are you a pointer is returned from one of the former functions where the contents have been free'd? If so, can we track that down first. If any error occurs, I think NULL should be returned from "Mat_VarReadNext" and "Mat_VarReadNextInfo".

Ultimately, I would also like to get rid of the Mat_Error, Mat_Critical, etc., however that would seem to require quite a bit of work.

[tbeu]

Please find my proposed patch attached.

[cch]

Thomas,
Can you explain why you removed setting matvar to NULL in matvar_struct.c? I think everything else in this patch seems ok. Specifically, the snippet below.

Chris

diff --git a/src/matvar_struct.c b/src/matvar_struct.c
index 9892f27..40c59e5 100644
--- a/src/matvar_struct.c
+++ b/src/matvar_struct.c
@@ -75,12 +75,10 @@ Mat_VarCreateStruct(const char name,int rank,size_t dims,const char fields,
malloc(nfieldssizeof(matvar->internal->fieldnames));
if ( NULL == matvar->internal->fieldnames ) {
Mat_VarFree(matvar);
- matvar = NULL;
} else {
for ( i = 0; i < nfields; i++ ) {
if ( NULL == fields[i] ) {
Mat_VarFree(matvar);
- matvar = NULL;
break;
} else {
matvar->internal->fieldnames[i] = strdup(fields[i]);

Just because matvar is now already nullified in Mat_VarFree. Thus second
nullify is superfluous and can be removed.

Thomas
-----Ursprungliche Nachricht-----
Von: cch [mailto:chulbe2lsu@users.sf.net]
Gesendet: Montag, 7. Juli 2014 22:52
An: [matio:bugs]
Betreff: [matio:bugs] #26 Dangerous error handling

Thomas,
Can you explain why you removed setting matvar to NULL in matvar_struct.c?
I think everything else in this patch seems ok. Specifically, the snippet
below.

Chris

diff --git a/src/matvar_struct.c b/src/matvar_struct.c
index 9892f27..40c59e5 100644
--- a/src/matvar_struct.c
+++ b/src/matvar_struct.c
@@ -75,12 +75,10 @@ Mat_VarCreateStruct(const char name,int rank,size_t
dims,const char fields,
malloc(nfieldssizeof(matvar->internal->fieldnames));
if ( NULL == matvar->internal->fieldnames ) {
Mat_VarFree(matvar);
- matvar = NULL;
} else {
for ( i = 0; i < nfields; i++ ) {
if ( NULL == fields[i] ) {
Mat_VarFree(matvar);
- matvar = NULL;
break;
} else {
matvar->internal->fieldnames[i] = strdup(fields[i]);

----------------------------------------------------------------------------

[bugs:#26] Dangerous error handling

Status: open
Group: master
Created: Wed Jun 04, 2014 02:51 PM UTC by tbeu
Last Updated: Mon Jun 23, 2014 02:50 PM UTC
Owner: nobody

The current error handling with calling Mat_VarFree immediately after
Mat_Error or Mat_Critical is dangerous. I noticed a crash of the matdump
tool if a severe error occurs during reading a MAT file. The matvar is then
freed first in the matio library and a second time in matdump.c (line 907 or
913). The simple reason for the crash is that the struct members of matvar
are not nullified in Mat_VarFree after freeing. Thus I propose to also
nullify freed variables.

----------------------------------------------------------------------------

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/matio/bugs/26/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

[cch]

But setting matvar to NULL in Mat_VarFree has nothing to do with the matvar pointer in the calling function such as Mat_VarCreateStruct which would get returned as non-NULL and point to free'd data. I will fix the patch and apply.

[tbeu]

Sorry, I still do not get it. Mat_VarFree takes pointer of matvar_t as input argument. After application of the proposed patch this pointer is set to NULL after freeing mem. Calling function (e.g. Mat_VarCreateStruct) calls Mat_VarFree with local variable of type matvar_t*. After returning from Mat_VarFree pointer is set to NULL. No need to nullify it again.

So why has matvar pointer of calling function nothing to do with matvar pointer of Mat_VarFree?

[cch]

Thomas,
Consider the simplified example below. The Mat_VarFree function is like the foo function below. Note that setting bar = NULL in foo has no effect in main. However, foo2 takes a double pointer and so that setting *bar = NULL has the desired effect.

[chulbert@maclap ~]$ cat test.c

#include <stdlib.h>
#include <stdio.h>

void foo(void *bar)
{
    free(bar);
    bar = NULL;
}

void foo2(void **bar)
{
    free(*bar);
    *bar = NULL;
}

int main(void)
{
    void *a = malloc(1);
    void *b = malloc(1);

    printf("before: %p %p\n",a,b);
    foo(a);
    foo2(&b);
    printf(" after: %p %p\n",a,b);
    return 0;
}

[chulbert@maclap ~]$ clang test.c
[chulbert@maclap ~]$ ./a.out
before: 0x7ff241c038a0 0x7ff241c038b0
 after: 0x7ff241c038a0 0x0

[tbeu]

Thanks for example. Still learning...

Do you want a new patch that keeps "matvar=NULL;" in calling function and removes it again from Mat_VarFree?

[tbeu]

It is a single line error only thus there is no need for a large patch: If inflateInit fails in Mat_VarReadNextInfo5 then after Mat_VarFree(matvar); matvar should be nullified.

[tbeu]

Fixed by 69c58ca.