MP4Close does not null out ptr argument

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Call MP4Open.  Get a valid mp4 file handle.
2. Call MP4Close with a given mp4 file handle.
3. File handle is not null, but the underlying ptr has been deleted by 
MP4Close.

What is the expected output? What do you see instead?

I think the file handle should be set to null after being deleted; this 
way callers can easily determine if a given handle is valid.

What version of the product are you using? On what operating system?

1.9.1, although the current issue exists in the trunk as well (seem 
MP4Close()

Please provide any additional information below.

Should be a one-line fix:

void MP4Close(MP4FileHandle hFile)     
{
...
delete &f;     
&f = NULL;    // <--NEW LINE OF CODE
} 

Original issue reported on code.google.com by kid...@gmail.com on 20 Aug 2009 at 7:20

[GoogleCodeExporter]

Ugg, sorry:

f = NULL;

No dereference.  my bad.

Original comment by kid...@gmail.com on 20 Aug 2009 at 7:21

[GoogleCodeExporter]

Whoa. That's wrong in so many ways.

As stated by point #3 you desire MP4FileHandle hFile to be set to NULL after 
object is destroyed.
That is not possible as hFile must be MP4FileHandle* in order to accomplish the 
enhancement you desire.
In case it was changed, then yes a simple "*hFile = NULL" would be your 
enhancement.
Unfortunately this will break all existing MP4Close() usage.

Secondly, it is illegal to assign NULL to an object, which 'f' represents. The 
reason why you do not get a 
compiler error is because MP4File(int) constructor accepts an integer. In my 
opinion it would be best to 
change MP4File(int) --> "explicit MP4File(int)" to avoid implicit conversion of 
NULL to a new object.

"f = NULL;" does the following which I'm sure is not desired:

1. create a temporary MP4File object by using constructor MP4File(int) which 
allows NULL to be used as per 
not perfectly clear C++ standard.
2. assign the tmp object to f (this uses default object copy semantics because 
there is no custom defined 
copy constructor)
3. this behavior would be undefined because 'f' was just deleted. while it 
might not generate a coredump, 
because the memory is most likely still available, it's certainly undefined to 
use a dead object with copy 
semantics.

Original comment by Kona8l...@gmail.com on 15 Sep 2009 at 11:45

[GoogleCodeExporter]

Small correction. It's strictly not "illegal" to assign NULL to an object. You 
need help from implicit constructors or 
overloaded assignment operators. In either case though, it's not what you 
desire to do for this issue.

Original comment by Kona8l...@gmail.com on 15 Sep 2009 at 11:48

[GoogleCodeExporter]

In my haste, I wrote some bad code.  Sorry.  Let me explain myself better.

Here is the specific function:

    void MP4Close(MP4FileHandle hFile)
    {
        if( !MP4_IS_VALID_FILE_HANDLE( hFile ))
            return;

        MP4File& f = *(MP4File*)hFile;
        try {
            f.Close();
        }
        catch ( MP4Error* e ) {
            PRINT_ERROR( e );
            delete e;
        }

        delete &f;
    }

MP4FileHandle is simply a void*:

typedef void*       MP4FileHandle;

...that is, just a pointer.  So the fix should be:

void MP4Close(MP4FileHandle hFile)     
{
...
delete &f;     
hFile = NULL;
} 

In general, when you delete something a pointer points to (and in this case, 
hFile
points to MP4File*), you should also set it to null.  Other places in the 
library do
this consistently, so I don't know why this function wouldn't.

Original comment by kid...@gmail.com on 16 Sep 2009 at 12:00

[GoogleCodeExporter]

Also, 

"*hFile = NULL"

...would be totally wrong.  This would actually do what you seem to be 
explaining,
which would be setting a class instance to NULL, which yes, I agree is a compile
error.  Again, I apologize for not being clear and posting some bad code, but 
this is
clearly not the point I was trying to make.

But the above change absolutely does not break any MP4Close() usage; it is 
perfectly
legal to NULL out a pointer to an object that has just been deleted.

Original comment by kid...@gmail.com on 16 Sep 2009 at 12:07

[GoogleCodeExporter]

I'll omit the typedef so you can see what's happening:

void MP4Close( void* pFile ) {
    pFile = NULL; // useless as pFile is an in-parm and would do nothing to set caller's pointer
}

void MP4Close( void** ppFile ) {
   *ppFile = NULL; // sets (out) pFile to NULL
}

And as mentioned earlier, this requires an incompatible change in function 
signature.

Original comment by Kona8l...@gmail.com on 16 Sep 2009 at 12:19

[GoogleCodeExporter]

Kona,

My apologies, the ** thing always throws me for a loop, but you're definitely 
right.
 And yes, I agree this would be a sizable change that is not worth the effort.  I'll
just null out the argument after I call MP4Close like I'm currently doing.

You can close this--thanks for the help!

Original comment by kid...@gmail.com on 16 Sep 2009 at 12:42

[GoogleCodeExporter]

Original comment by eddyg.o....@gmail.com on 22 Nov 2009 at 11:39

• Changed state: WontFix