RE: RFR: 8205342: windows : potential memleaks in getAdapter(s) in NetworkInterface_winXP.c

[Baesken, Matthias]

Hello Alan, Thomas ,  I adjusted the line lengths  and created a new webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8205342.1/

I considered  replacing the  100   for error_msg_buf  size  by a define  (or 
maybe const  int?)  , should I do so ?


Best regards, Matthias


From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Mittwoch, 20. Juni 2018 10:45
To: Baesken, Matthias ; net-dev@openjdk.java.net
Subject: Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in 
NetworkInterface_winXP.c


On 20/06/2018 09:07, Baesken, Matthias wrote:
Hello . Please review this small  fix that  fixes  potential  memory leaks in   
getAdapter(s) in NetworkInterface_winXP.c  and simplifies the coding a bit too .
Currently   when generating error messages ,   some memory  is malloc-ed  for 
the error messages , but not always freed .


Bug:

https://bugs.openjdk.java.net/browse/JDK-8205342

webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8205342/

Can you fix the line lengths to make it consistent with original code? That 
will make it easier to look at side-by-side diffs.

-Alan

Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in NetworkInterface_winXP.c

[Ivan Gerasimov]

Hello Matthias!

Thanks for the fix!


On 6/22/18 6:08 AM, Baesken, Matthias wrote:


Hello Alan, Thomas ,  I adjusted the line lengths  and created a new 
webrev :


http://cr.openjdk.java.net/~mbaesken/webrevs/8205342.1/ 



I considered  replacing the  100   for error_msg_buf  size  by a 
define  (or maybe const  int?)  , should I do so ?


I'd prefer to have hardcoded 100 replaced with sizeof(error_msg_buf) at 
lines 125 and 195.
And with sizeof(error_msg_buf) / sizeof(error_msg_buf[0]) at lines 126 
and 196.


I understand that it is highly unlikely that type of error_msg_buf will 
ever change, but I think it would express the intention for the argument 
values clearer.


With kind regards,
Ivan


Best regards, Matthias

*From:*Alan Bateman [mailto:alan.bate...@oracle.com]
*Sent:* Mittwoch, 20. Juni 2018 10:45
*To:* Baesken, Matthias ; 
net-dev@openjdk.java.net
*Subject:* Re: RFR: 8205342: windows : potential memleaks in 
getAdapter(s) in NetworkInterface_winXP.c


On 20/06/2018 09:07, Baesken, Matthias wrote:

Hello . Please review this small  fix that  fixes  potential 
memory leaks in   getAdapter(s) in NetworkInterface_winXP.c and

simplifies the coding a bit too .

Currently   when generating error messages ,   some memory  is
malloc-ed for the error messages , but not always freed .

Bug:

https://bugs.openjdk.java.net/browse/JDK-8205342

webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8205342/


Can you fix the line lengths to make it consistent with original code? 
That will make it easier to look at side-by-side diffs.


-Alan



--
With kind regards,
Ivan Gerasimov

Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in NetworkInterface_winXP.c

[Thomas Stüfe]

Hi Matthias,

On Fri, Jun 22, 2018 at 3:08 PM, Baesken, Matthias
 wrote:
> Hello Alan, Thomas ,  I adjusted the line lengths  and created a new webrev
> :
>
>
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205342.1/
>
>
>
> I considered  replacing the  100   for error_msg_buf  size  by a define  (or
> maybe const  int?)  , should I do so ?
>

You may be able to do this:

#define ERRMSG"IP Helper Library GetAdaptersAddresses"
#define ERRMSG_LEN   sizeof(ERRMSG)
#define ADD_LEN   30
char error_msg_buf[ERRMSG_LEN + ADD_LEN];

since sizeof(string literal) should be resolved by the preprocessor.

If that does not work, a macro as you suggested:

#define ERRMSG"IP Helper Library GetAdaptersAddresses"
#define ERRMSG_LEN   38
#define ADD_LEN   30
char error_msg_buf[ERRMSG_LEN + ADD_LEN];

Remarks:

- The memset is not needed

- As Ivan mentioned, please use sizeof(buf) instead of 100 in the call
to sprintf. IMHO however sizeof(buf)/sizeof(buf[0]) is not necessary:
we use this pattern (sizeof(buf) assuming buf = char = size 1 for
buffer output) all over the hotspot and jdk already, so if this is a
problem, we should change all those places too.

- Since your buffer will be always large enough to hold the result,
you can remove truncation handling altogether.

- I would change _snprint_s to plain snprintf because who on earth
knows what _snprintf_s() is :-)

- Matter of fact, since we cannot get truncation you may even switch
to sprintf - but that would probably make coverity complain out again.

That reduces the code to:

#define ERRMSG"IP Helper Library GetAdaptersAddresses"
#define ERRMSG_LEN   38
#define ADD_LEN   30
char error_msg_buf[ERRMSG_LEN + ADD_LEN];

snprintf_s(error_msg_buf, sizeof(error_msg_buf), "IP Helper Library
GetAdaptersAddresses "
 "function failed with error == %d", ret);

JNU_ThrowByName(env, "java/lang/Error", error_msg_buf);


---

Beside all that I still think that the least invasive and most clear
patch by far for your problem - the memory leak - would have been to
just add the two missing free() calls...

Best Regards, Thomas



>
>
>
>
> Best regards, Matthias
>
>
>
>
>
> From: Alan Bateman [mailto:alan.bate...@oracle.com]
> Sent: Mittwoch, 20. Juni 2018 10:45
> To: Baesken, Matthias ; net-dev@openjdk.java.net
> Subject: Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in
> NetworkInterface_winXP.c
>
>
>
>
>
> On 20/06/2018 09:07, Baesken, Matthias wrote:
>
> Hello . Please review this small  fix that  fixes  potential  memory leaks
> in   getAdapter(s) in NetworkInterface_winXP.c  and simplifies the coding a
> bit too .
>
> Currently   when generating error messages ,   some memory  is malloc-ed
> for the error messages , but not always freed .
>
>
>
>
>
> Bug:
>
>
>
> https://bugs.openjdk.java.net/browse/JDK-8205342
>
>
>
> webrev :
>
>
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205342/
>
>
>
> Can you fix the line lengths to make it consistent with original code? That
> will make it easier to look at side-by-side diffs.
>
> -Alan