[ECOS] Bug in getifaddrs() - allocation of buffer

[ECOS] Bug in getifaddrs() - allocation of buffer

[Jay Foster]

The function getifaddrs() in file packages/net/common/current/src/ifaddrs.c
has a couple of bugs in it.

On lines 151 and 157, it calls "free(buf)", but "buf" is a local buffer
allocated on the stack.  These should either be removed, or change the 
allocation of "buf" to be malloced.  These free() calls seem to indicate
that at one time, the "buf" buffer was malloced, which makes sense, since
it is a 1K byte buffer.  This seems a bit large for an auto stack variable.

On lines 172 through 177 (CYGPKG_NET_INET6), it returns from the function
(error case), but does not free the malloced buffer "data".  Need to add
a call to "free(data);" here.  Also, if the "buf" buffer allocation is 
changed to be malloced, instead of on the stack, then add a "free(buf);"
here too.

I will submit a patch, but I wanted to get some feedback/discussion about
the allocation of the "buf" buffer (malloced vs. on the stack).


Jay


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] Bug in getifaddrs() - allocation of buffer

[Andrew Lunn]

On Tue, Sep 16, 2003 at 08:18:11AM -0700, Jay Foster wrote:
> The function getifaddrs() in file packages/net/common/current/src/ifaddrs.c
> has a couple of bugs in it.
> 
> On lines 151 and 157, it calls "free(buf)", but "buf" is a local buffer
> allocated on the stack.  These should either be removed, or change the 
> allocation of "buf" to be malloced.  These free() calls seem to indicate
> that at one time, the "buf" buffer was malloced, which makes sense, since
> it is a 1K byte buffer.  This seems a bit large for an auto stack variable.

I agree, it should be malloc'd. Its been broken since at least
20-May-02!

> 
> On lines 172 through 177 (CYGPKG_NET_INET6), it returns from the function
> (error case), but does not free the malloced buffer "data".  Need to add
> a call to "free(data);" here.  Also, if the "buf" buffer allocation is 
> changed to be malloced, instead of on the stack, then add a "free(buf);"
> here too.

Agreed.

        Andrew

-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] Bug in getifaddrs() - allocation of buffer

[Gary Thomas]

On Tue, 2003-09-16 at 10:37, Andrew Lunn wrote:
> On Tue, Sep 16, 2003 at 08:18:11AM -0700, Jay Foster wrote:
> > The function getifaddrs() in file packages/net/common/current/src/ifaddrs.c
> > has a couple of bugs in it.
> > 
> > On lines 151 and 157, it calls "free(buf)", but "buf" is a local buffer
> > allocated on the stack.  These should either be removed, or change the 
> > allocation of "buf" to be malloced.  These free() calls seem to indicate
> > that at one time, the "buf" buffer was malloced, which makes sense, since
> > it is a 1K byte buffer.  This seems a bit large for an auto stack variable.
> 
> I agree, it should be malloc'd. Its been broken since at least
> 20-May-02!
> 

Actually, it's been broken (in this respect) forever :-(  This is how 
the code looked when originally imported.

> > 
> > On lines 172 through 177 (CYGPKG_NET_INET6), it returns from the function
> > (error case), but does not free the malloced buffer "data".  Need to add
> > a call to "free(data);" here.  Also, if the "buf" buffer allocation is 
> > changed to be malloced, instead of on the stack, then add a "free(buf);"
> > here too.
> 
> Agreed.

Yes, a little looking at this file and how those pointers [objects] are
allocated and freed would be a good idea.

-- 
Gary Thomas <gary@mlbassoc.com>
MLB Associates


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

RE: [ECOS] Bug in getifaddrs() - allocation of buffer

[Jay Foster]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

Attached is a patch to tidy this up.

Jay


-----Original Message-----
From: Gary Thomas [mailto:gary@mlbassoc.com]
Sent: Tuesday, September 16, 2003 9:56 AM
To: Andrew Lunn
Cc: Jay Foster; 'ecos-discuss@sources.redhat.com'
Subject: Re: [ECOS] Bug in getifaddrs() - allocation of buffer


On Tue, 2003-09-16 at 10:37, Andrew Lunn wrote:
> On Tue, Sep 16, 2003 at 08:18:11AM -0700, Jay Foster wrote:
> > The function getifaddrs() in file
packages/net/common/current/src/ifaddrs.c
> > has a couple of bugs in it.
> > 
> > On lines 151 and 157, it calls "free(buf)", but "buf" is a local buffer
> > allocated on the stack.  These should either be removed, or change the 
> > allocation of "buf" to be malloced.  These free() calls seem to indicate
> > that at one time, the "buf" buffer was malloced, which makes sense,
since
> > it is a 1K byte buffer.  This seems a bit large for an auto stack
variable.
> 
> I agree, it should be malloc'd. Its been broken since at least
> 20-May-02!
> 

Actually, it's been broken (in this respect) forever :-(  This is how 
the code looked when originally imported.

> > 
> > On lines 172 through 177 (CYGPKG_NET_INET6), it returns from the
function
> > (error case), but does not free the malloced buffer "data".  Need to add
> > a call to "free(data);" here.  Also, if the "buf" buffer allocation is 
> > changed to be malloced, instead of on the stack, then add a "free(buf);"
> > here too.
> 
> Agreed.

Yes, a little looking at this file and how those pointers [objects] are
allocated and freed would be a good idea.

-- 
Gary Thomas <gary@mlbassoc.com>
MLB Associates


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss


[-- Attachment #2: ifaddrs.pat --]
[-- Type: application/octet-stream, Size: 3063 bytes --]

Index: packages/net/common/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/common/current/ChangeLog,v
retrieving revision 1.40
diff -u -5 -p -r1.40 ChangeLog
--- packages/net/common/current/ChangeLog	31 Jul 2003 08:56:26 -0000	1.40
+++ packages/net/common/current/ChangeLog	16 Sep 2003 21:36:18 -0000
@@ -1,5 +1,10 @@
+2003-09-16  Jay Foster  <jay@systech.com>
+ 
+ 	* src/ifaddrs.c (getifaddrs): Fix up allocation and freeing of
+    work buffers and data buffers.
+ 
 2003-07-29  Motoya Kurotsu  <kurotsu@allied-telesis.co.jp>
 
         * src/dhcp_prot.c (do_dhcp): (re)Initialize the lease when
         dhcp is (re)initialized or retried.
         Return true when the state falls into DHCPSTATE_NOTBOUND so that it 
Index: packages/net/common/current/src/ifaddrs.c
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/common/current/src/ifaddrs.c,v
retrieving revision 1.4
diff -u -5 -p -r1.4 ifaddrs.c
--- packages/net/common/current/src/ifaddrs.c	12 May 2003 10:58:10 -0000	1.4
+++ packages/net/common/current/src/ifaddrs.c	16 Sep 2003 21:36:18 -0000
@@ -102,11 +102,12 @@ int
 getifaddrs(struct ifaddrs **pif)
 {
     int icnt = 1;  // Interface count
     int dcnt = 0;  // Data [length] count
     int ncnt = 0;  // Length of interface names
-    char buf[1024];
+    char *buf;
+#define	IF_WORK_SPACE_SZ	1024
     int i, sock;
 #ifdef CYGPKG_NET_INET6
     int sock6;
     struct in6_ifreq ifrq6;
 #endif
@@ -114,19 +115,26 @@ getifaddrs(struct ifaddrs **pif)
     struct ifreq *ifr, *lifr;
     struct ifreq ifrq;
     char *data, *names;
     struct ifaddrs *ifa, *ift;
 
+    buf = malloc(IF_WORK_SPACE_SZ);
+    if (buf == NULL)
+        return (-1);
     ifc.ifc_buf = buf;
-    ifc.ifc_len = sizeof(buf);
+    ifc.ifc_len = IF_WORK_SPACE_SZ;
 
     if ((sock = socket(AF_INET, SOCK_STREAM, 0)) < 0)
+    {
+        free(buf);
         return (-1);
+    }
     i =  ioctl(sock, SIOCGIFCONF, (char *)&ifc);
 
     if (i < 0) {
         close(sock); 
+        free(buf);
         return (-1);
     }
 
     ifr = ifc.ifc_req;
     lifr = (struct ifreq *)&ifc.ifc_buf[ifc.ifc_len];
@@ -165,14 +173,15 @@ getifaddrs(struct ifaddrs **pif)
 
     memset(ifa, 0, sizeof(struct ifaddrs) * icnt);
     ift = ifa;
 
     ifr = ifc.ifc_req;
-    lifr = (struct ifreq *)&ifc.ifc_buf[ifc.ifc_len];
 
 #ifdef CYGPKG_NET_INET6
     if ((sock6 = socket(AF_INET6, SOCK_STREAM, 0)) < 0) {
+      free(buf);
+      free(data);
       close(sock);
       return (-1);
     }
 #endif
 
@@ -232,10 +241,11 @@ getifaddrs(struct ifaddrs **pif)
             ifr = (struct ifreq *)(((char *)sa) + sizeof(*sa));
         else
             ifr = (struct ifreq *)(((char *)sa) + SA_LEN(sa));
         ift = (ift->ifa_next = ift + 1);
     }
+    free(buf);
 
     if (--ift >= ifa) {
         ift->ifa_next = NULL;
         *pif = ifa;
     } else {


[-- Attachment #3: Type: text/plain, Size: 146 bytes --]

-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss