Memory leak in Ethernet.cpp (allocating DHCP object)

[GoogleCodeExporter]

What steps will reproduce the problem?

1. Call EthernetClass::begin() multiple times. Get memory leak.

What is the expected output? What do you see instead?

The code does this:

int EthernetClass::begin(uint8_t *mac_address)
{
  _dhcp = new DhcpClass();

However it makes no attempt to delete or re-use the _dhcp element, thus giving 
a memory leak if the caller does begin() multiple times.

Suggest either:

  delete _dhcp;
  _dhcp = new DhcpClass();

or:

  if (_dhcp == NULL)    
    _dhcp = new DhcpClass();  

Also set _dhcp to NULL initially which does not appear to be done currently. 
This is a pity because other code tests if it is NULL.

What version of the Arduino software are you using?

1.0.1

On what operating system?  Which Arduino board are you using?

Not really relevant.

Please provide any additional information below.

See:

http://arduino.cc/forum/index.php?topic=108013

Original issue reported on code.google.com by n...@gammon.com.au on 30 May 2012 at 11:10

[GoogleCodeExporter]

Adrian, can you take a look at this one?

Original comment by dmel...@gmail.com on 1 Jun 2012 at 3:17

• Changed state: Accepted
• Added labels: Component-Core, Component-Ethernet

[GoogleCodeExporter]

Now that I look more closely, should not:

  _dhcp = new DhcpClass();

be:

  _dhcp = new DhcpClass;

Original comment by n...@gammon.com.au on 1 Jun 2012 at 3:32

[GoogleCodeExporter]

I found the same bug. I filed a duplicate (Id #1000) before I noticed that 
someone else had already found the same problem.

I prefer the first fix using delete because you don't have to worry if there 
might be some side effects from reusing _dhcp.

  delete _dhcp;
  _dhcp = new DhcpClass();

It would be nice if the original poster submitted a patch to include the change 
that initializes _dhcp to NULL.

I have attached my test sketch which demonstrates the problem. After about 15 
to 20 loops the Arduino will either hang or reset itself.

Original comment by n...@noah.org on 31 Jul 2012 at 10:07

Attachments:

• dhcp_stress_test.ino

[GoogleCodeExporter]

Good spot, and the fix looks like it would work.

It has tempted me back to wondering if we could get away without creating the 
DhcpClass object on the heap again though.  Allocating it on the heap uses an 
extra 500-plus bytes in the sketch, as it has to pull in new/delete (although 
if you're using them elsewhere you'd have that anyway).

I wonder if we could break the DhcpClass into two parts: DhcpClass which would 
do the work of requesting leases, etc. and which would only be created while 
the Arduino needed to talk to a DHCP server; and a DhcpPersistentClass, which 
would hold the lease info and be contained within the Ethernet class.

That solution would mean we didn't need new/delete (saving 500+ bytes of sketch 
size) and would use 81 fewer bytes of RAM apart from when it was actually 
acquiring a lease.

Given that I'm looking at the DHCP stuff for issue 985 too, I'll have a look at 
changing this too, unless anyone can think of a reason not to?

Original comment by adrian.m...@gmail.com on 10 Aug 2012 at 4:03

[GoogleCodeExporter]

It looks like the DhcpClass will need a destructor if the "delete _dhcp; _dhcp 
= new DhcpClass();" option is chosen, as its _dhcpUdpSocket will need to be 
closed.

Original comment by richard....@gmail.com on 3 Dec 2012 at 2:32

[GoogleCodeExporter]

I'd like to suggest implementing this as Ethernet.stop(), to match several 
other APIs that have begin() and stop() methods.

I don't think I'd consider this a breaking API change, since if you don't call 
stop() the behaviour would be identical to what it is now.

Original comment by richard....@gmail.com on 3 Dec 2012 at 5:16

[GoogleCodeExporter]

I have submitted a pull request with a fix for this issue here: 
https://github.com/arduino/Arduino/pull/1150

Original comment by richard....@gmail.com on 3 Dec 2012 at 8:01

[GoogleCodeExporter]

Merged, thank you.

https://github.com/arduino/Arduino/commit/cce70d269c23e9985fe970378fb6dd209b5766
43

Original comment by c.mag...@bug.st on 12 Dec 2012 at 12:02

• Changed state: Verified

[GoogleCodeExporter]

Issue 1000 has been merged into this issue.

Original comment by c.mag...@bug.st on 15 Dec 2012 at 11:57