Pages: [1]   Go Down
Author Topic: Ethernet Memory Leak --this thread has been replaced  (Read 1287 times)
0 Members and 1 Guest are viewing this topic.
0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 67
A couple of shields away from a w*******g robot
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I wrote some code using Visual C++ before putting it on the arduino (+ ethernet shield).

It worked in Visual Studio but when I put it on the arduino I got no response. I then removed a couple of functions to simplify it (hence the returns at the beginning of a couple of functions).... it now returns a page. But there is a memory issue as after the first call I get weird results.

Perhaps someone can tell me where this might occur. I am new to C++ (though work as a SQL/VB developer) so any useful critisism will be gladly taken on board. I am currently trying to get my head around the way c++ deals with memory.

Thanks

G

* WebServer.zip (4.08 KB - downloaded 14 times.)
« Last Edit: March 20, 2011, 03:34:12 pm by Beakie » Logged

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 67
A couple of shields away from a w*******g robot
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I forgot to say that the code takes a web page request and parses it before returning a page. I think it will be to do with the HTTPRequest struct... but u tell me
Logged

Seattle, WA USA
Online Online
Brattain Member
*****
Karma: 654
Posts: 50928
Seattle, WA USA
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
  char cReturn[10000];
None of the Arduinos has enough SRAM for a 10000 byte array.
Logged

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 67
A couple of shields away from a w*******g robot
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

None of the Arduinos has enough SRAM for a 10000 byte array.

Quite right. Thanks for that.

I changed all the arrays to much smaller arrays so I don't exceed the 1024 max sram allowance... but it still doesn't work. And it fails in exactly the same way. The first 5 requests for pages work but the next few return dodgy results.

Anything else you feel could be causing the problem?

smiley
Logged

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 227
Posts: 14048
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Do you have code to share? might be helpfull
Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 67
A couple of shields away from a w*******g robot
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Do you have code to share? might be helpfull

The code is attached to my first post... but here is the latest version.


* WebServer.zip (4.1 KB - downloaded 11 times.)
Logged

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 227
Posts: 14048
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

missed that, sorry.

-- update --

a quick scan:

Code:
char* HTMLEncode(char* _Text)
{
  char cReturn[200];
  unsigned int iReturn = 0;
  for (int iLoop; iLoop <= strlen(_Text); iLoop ++)
  {
    switch(_Text[iLoop])
    {
      case '\n':
        cReturn[iReturn] = '<'; iReturn++;
        cReturn[iReturn] = 'b'; iReturn++;
        cReturn[iReturn] = 'r'; iReturn++;
        cReturn[iReturn] = '/'; iReturn++;
        cReturn[iReturn] = '>'; iReturn++;
        break;
      case '<':
        cReturn[iReturn] = '&'; iReturn++;
        cReturn[iReturn] = 'l'; iReturn++;
        cReturn[iReturn] = 't'; iReturn++;
        cReturn[iReturn] = ';'; iReturn++;
        break;
      case '>':
        cReturn[iReturn] = '&'; iReturn++;
        cReturn[iReturn] = 'g'; iReturn++;
        cReturn[iReturn] = 't'; iReturn++;
        cReturn[iReturn] = ';'; iReturn++;
        break;
      case '"':
        cReturn[iReturn] = '&'; iReturn++;
        cReturn[iReturn] = 'q'; iReturn++;
        cReturn[iReturn] = 'u'; iReturn++;
        cReturn[iReturn] = 'o'; iReturn++;
        cReturn[iReturn] = 't'; iReturn++;
        cReturn[iReturn] = ';'; iReturn++;
        break;
      case '&':
        cReturn[iReturn] = '&'; iReturn++;
        cReturn[iReturn] = 'a'; iReturn++;
        cReturn[iReturn] = 'm'; iReturn++;
        cReturn[iReturn] = 'p'; iReturn++;
        cReturn[iReturn] = ';'; iReturn++;
        break;
      case ' ':
        cReturn[iReturn] = '&'; iReturn++;
        cReturn[iReturn] = 'n'; iReturn++;
        cReturn[iReturn] = 'b'; iReturn++;
        cReturn[iReturn] = 's'; iReturn++;
        cReturn[iReturn] = 'p'; iReturn++;
        cReturn[iReturn] = ';'; iReturn++;
        break;
      default:
        cReturn[iReturn] = _Text[iLoop];
        iReturn++;
        break;
    }
  }
  return cReturn;
}
Above function is faulty. It returns a pointer to a local variable of a function. After the call to this function the stack/heap space will be overwritten by other functions etc so the pointer points to something undefined at best smiley-sad

Code:
void AddNameValue(char* Key, char* Value, char*** KeyArray, char*** ValueArray, int* ArrayLength)
{
  char** tmpKeys = (char**)realloc(*KeyArray, (*ArrayLength + 1) * sizeof(char*));
  char** tmpValues = (char**)realloc(*ValueArray, (*ArrayLength + 1) * sizeof(char*));

  if (tmpKeys)
  {
    *KeyArray = tmpKeys;
  }
  if (tmpValues)
  {
    *ValueArray = tmpValues;
  }
// if (!keys || !values)
// return false; // out of memory

  (*KeyArray)[*ArrayLength] = strdup(Key);
  (*ValueArray)[*ArrayLength] = strdup(Value);
  ++(*ArrayLength);
}
Why are you using two  char *** params  (I've only used three levels of indirection at school)   Can you explain that?

Code:
  void HandleQueryStringLine(char* _QueryStringLine)
  {
return;
...
}
This function (normally) uses two large arrays but after the call these arrays are gone as they are local variables.

Code:
void DoEthernet(void (*f)(Client, HTTPRequest))
{
  Client HTTPClient = server.available();
  unsigned int iInputIndex = 0;
  char cReceived[300];
 
  if (HTTPClient)
  {
    boolean currentLineIsBlank = true;
   
    while (HTTPClient.connected())
    {
      if (HTTPClient.available())
      {
        char c = HTTPClient.read();

        cReceived[iInputIndex] = c;
        iInputIndex++;

        if (c == '\n' && currentLineIsBlank)
        {
          HTTPRequest h; //should i do a free here and malloc at char cReceived[..]? Would need init to do malloc at beginning to stop memory error???
          h.Init();
          h.Parse(cReceived);
          f(HTTPClient, h);
         
          break;
        }

        if (c == '\n')
        {
          currentLineIsBlank = true;
        }
        else
        {
          if (c != '\r') { currentLineIsBlank = false; }
        }
      }
    }

    delay(1);
    HTTPClient.stop();
  }
}

        if (c == '\n' && currentLineIsBlank)

If there is text on a line currentLineis Blank will be made false and the if will never be true again. (or??)

=========

Conclusion - after a quick scan and compile (not runned the code):

- I cannot get an overview from the code how it works (or is intended). For me that implies the architecture is complex (= my opinion).

- You do very much (imho too much) with pointers, that is ok but you have to take care that you free the allocated memory. Especially before reassigning new values to a pointer to a pointer .... Advice: - rewrite the code with as less pointers as possible - e.g. use fixed global array's, that makes the function calls far simpler ...

« Last Edit: March 20, 2011, 10:54:47 am by robtillaart » Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 67
A couple of shields away from a w*******g robot
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

missed that, sorry.


s'all good. please lemme know if u see anything wrong
Logged

UK
Offline Offline
Full Member
***
Karma: 2
Posts: 110
Kittens eat Arduinos
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

strdup() wraps up memory allocation and copying..

Your struct Page has strdup() in its constructor (page.h), and no destructor - so these are memory leaks.

Your DoResponse(...) (WebServer.pde) creates a Page and uses strdup() to also leak memory.

So your program will crash quite quickly.

You have two alternatives
1 - do not use strdup(), just assign the string to the pointer.
2 - or make sure you free the old pointer before replacing it, and free it in the destructor.

--
There is a lot of code, and I am not familiar with the environment, so this is probably not all of the problems.

 

Logged

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 67
A couple of shields away from a w*******g robot
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Thanks for your input. Considering what you said...

Question 1:
Looking at the code below. If I set PageTitle as "Title" (PageTitle being a char* within the struct) will c++ allocate the space to fit these 5 characters in for me? Do I need to free this space afterwards if I then set it to something else?

Code:

void DoResponse(Client _HTTPClient, HTTPRequest _Request)
{
  Page xPage;
  xPage.Init();

  xPage.PageTitle = "Title";

  xPage.Render(_HTTPClient);
}


Question 2:
The function that encodes for HTML. How SHOULD I achieve what I want?
I want to pass in a pointer to a string, create a space to put a new encoded string in (leaving the original untouched), and then return a pointer to that new string.

If I create the array like below, the memory will be destroyed after the function ends (right?) so how do I pass this back without requiring a call to free().

Code:
char* HTMLEncode(char* _Text)
{
  char cReturn[200];
  //.....
}

Thanks
Logged

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 67
A couple of shields away from a w*******g robot
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset


Above function is faulty. It returns a pointer to a local variable of a function. After the call to this function the stack/heap space will be overwritten by other functions etc so the pointer points to something undefined at best smiley-sad


OK


Why are you using two  char *** params  (I've only used three levels of indirection at school)   Can you explain that?


I was trying to pass a pointer to an "array of strings".


Conclusion - after a quick scan and compile (not runned the code):

- I cannot get an overview from the code how it works (or is intended). For me that implies the architecture is complex (= my opinion).

- You do very much (imho too much) with pointers, that is ok but you have to take care that you free the allocated memory. Especially before reassigning new values to a pointer to a pointer .... Advice: - rewrite the code with as less pointers as possible - e.g. use fixed global array's, that makes the function calls far simpler ...



Thanks for your input. I appreciate this code might be a little complicated for someone asking basic memory questions/making basic memory errors but I am a big believer in jumping in the deep end with stuff like this. I stand by my design just not the code to do it smiley
« Last Edit: March 20, 2011, 02:16:59 pm by Beakie » Logged

0
Offline Offline
Jr. Member
**
Karma: 0
Posts: 67
A couple of shields away from a w*******g robot
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I realised this code was too complicated so I simplified an example here...

http://arduino.cc/forum/index.php/topic,55969.0.html

Please comment there instead. It should make things easier.
Logged

Pages: [1]   Go Up
Jump to: