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
<matthias.baesken at sap.com> 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.Bateman at oracle.com]
> Sent: Mittwoch, 20. Juni 2018 10:45
> To: Baesken, Matthias <matthias.baesken at sap.com>; net-dev at 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