Memory leak in JNI function Java_java_io_WinNTFileSystem_rename0?

Question

I think the following function in the file jdk/src/windows/native/java/io/WinNTFileSystem_md.c in http://download.java.net/openjdk/jdk6/promoted/b27/openjdk-6-src-b27-26_oct_2012.tar.gz neglects to free the memory used by frompath or topath if one of them is found to be NULL...

JNIEXPORT jboolean JNICALL
Java_java_io_WinNTFileSystem_rename0(JNIEnv *env, jobject this, jobject from,
                                 jobject to)
{

    jboolean rv = JNI_FALSE;
    WCHAR *frompath = fileToNTPath(env, from, ids.path);
    WCHAR *topath = fileToNTPath(env, to, ids.path);
    if (frompath == NULL || topath == NULL)
        return JNI_FALSE;
    if (_wrename(frompath, topath) == 0) {
        rv = JNI_TRUE;
    }
    free(frompath);
    free(topath);
    return rv;
}

Am I missing something? Is this in fact a bug?

Resolved: Looking further into the details of the function pathToNTPath in io_util_md.c, I can see that fileToNTPath will only return NULL in the case of an out-of-memory error, so I guess we don't care if we neglect to free something we malloced when the JVM is about to crash! This still should be documented in the Java_java_io_WinNTFileSystem_rename0 function in my opinion though.

But what if one of them is NULL and the other isn't? Then the non-NULL one won't get freed. — Jesse, Apr 07 '13 at 09:18
If you free a null in C nothing will happen no exception nothing. http://www.open-std.org/JTC1/SC22/wg14/www/docs/n1124.pdf So if one of them is null the next line will be executed... — cgon, Apr 07 '13 at 09:22
http://stackoverflow.com/questions/1938735/does-freeptr-where-ptr-is-null-corrupt-memory — cgon, Apr 07 '13 at 09:23
And I am pretty sure this function has been tested by openjdk team. — cgon, Apr 07 '13 at 09:25
No, this method will never try to free a NULL. It will return without freeing a non-NULL when one of frompath and topath is NULL and the other isn't. Please check the code carefully. — Jesse, Apr 07 '13 at 09:26

score 1 · Answer 1 · answered Apr 07 '13 at 11:27

I think the original point is valid. While the research into the other function used by this code does suggest that the problem might not be that important, this code is lacking when viewed in and of itself.

The general rule of code reviews is that if someone has a question, it should generally be answered in the code with at least a comment.

The general rule of comments is that if it can be expressed in code, it possibly should be.

All of these issues go away if the code is written to eliminate the question.

score 0 · Answer 2 · answered Apr 07 '13 at 10:19

0

From what I can see from pathToNTPath(), which is called by fileToNTPath(), it returns NULL only in case of OutOfMemoryException, so I guess it's safe to assume that one does not need to take much care about freeing a few bytes of path name.

answered Apr 07 '13 at 10:19

Denis Tulskiy

19,012
6
50
68

Jesse · Accepted Answer · 2013-04-08T10:59:42.087

0

Resolved: Looking further into the details of the function pathToNTPath in io_util_md.c, I can see that fileToNTPath will only return NULL in the case of an out-of-memory error, so I guess we don't care if we neglect to free something we malloced when the JVM is about to crash! This still should be documented in the Java_java_io_WinNTFileSystem_rename0 function in my opinion though.

edited Apr 08 '13 at 10:59

answered Apr 08 '13 at 08:53

Jesse

11
3

Memory leak in JNI function Java_java_io_WinNTFileSystem_rename0?

3 Answers3