Re: RFR: JDK-8007606

[John Zavgren]

Greetings:
I fixed numerous errors that were identified by Mr. Davidovich.
I've posed a new webrev image at: 
http://cr.openjdk.java.net/~jzavgren/8007606/webrev.02/ 
<http://cr.openjdk.java.net/%7Ejzavgren/8007606/webrev.02/>

Thanks!
John Zavgren
On 02/05/2013 07:33 PM, Vitaly Davidovich wrote:

Hi John,

In NetworkInterface.c:

162           ret = (MIB_IFROW *) malloc(sizeof(MIB_IFROW));
163           if(ret == NULL)
164               return NULL;

tableP is left dangling if line 164 runs, no?

222     if (ret != NO_ERROR) {
223         if (tableP != NULL) {
224             free(tableP);

225             JNU_ThrowByName(env, "java/lang/Error",
226                      "IP Helper Library GetIfTable function failed");
227             JNU_ThrowOutOfMemoryError(env, "Native heap allocation 
failure");
228             return -1;
229         }

How do you know it's an OOM here?

289         curr = (netif *)calloc(1, sizeof(netif));
290         if (curr != NULL) {
291             wlen = MultiByteToWideChar(CP_OEMCP, 0, ifrowP->bDescr,
292                        ifrowP->dwDescrLen, NULL, 0);
293             if(wlen == 0) {
294                 // MultiByteToWideChar should not fail
295                 // But in rare case it fails, we allow 'char' to 
be displayed
296                 curr->displayName = (char 
*)malloc(ifrowP->dwDescrLen + 1);
297                 if(curr->displayName == NULL)
298                     return -1;
299
300             } else {
301                 curr->displayName = (char 
*)malloc(wlen*(sizeof(wchar_t))+1);
302                 if(curr->displayName == NULL)
303                     return -1;
304             }

Before returning, doesn't curr itself need to be freed? Same thing in 
this block:

308             if (curr->name == NULL || curr->displayName == NULL) {
309                 if (curr->name) free(curr->name);
310                 if (curr->displayName) free(curr->displayName);
311                 curr = NULL;
312             }

In NetworkInterface_winXP.c:

154         if(ret == NULL) {
155             JNU_ThrowByName(env, "java/lang/OutOfMemoryError", 0);
156             return NULL;
157         }

adapterInfo needs free here?

I also noticed some ptrs are compared against NULL while others 
against 0 - why the inconsistency?

Also, might it be cleaner to use multiple chained goto's to handle 
cleanup in functions with multiple allocations? The early returns in 
some of these places are a recipe for leaks I think.  I'm thinking 
something like:

r1 = malloc(...);
if (r1 == NULL) goto out1;

r2 = malloc(...);
if (r2 == NULL) goto out2;

out2:
    free(r2);
out1:
    free(r1);

Thanks

Sent from my phone

On Feb 5, 2013 6:42 PM, "John Zavgren" <john.zavg...@oracle.com 
<mailto:john.zavg...@oracle.com>> wrote:

    Greetings:
    I modified the use of malloc() and realloc() so that return values
    are checked and potential memory leaks and data corruption
    problems are prevented.

    http://cr.openjdk.java.net/~jzavgren/8007606/webrev.01/
    <http://cr.openjdk.java.net/%7Ejzavgren/8007606/webrev.01/>

    Thanks!
    John Zavgren



--
John Zavgren
john.zavg...@oracle.com
603-821-0904
US-Burlington-MA