sirleech/Webduino

inadequate use of pointer leading to crash

Closed this issue · 1 comments

This is a nice library, but it caused me some headache. In your code you are passing pointer to ints and that you manipulate the value (line 1183).

void WebServer::getRequest(WebServer::ConnectionType &type, char *request, int *length) {
    --*length; // save room for NUL

I don't get the point of passing a pointer to the int, why not just pass by value, if you are modifying the value (or by reference, if it's not modified)? So you end up in manipulating the value of a variable outside the scope of the function. This is bad an and causes the following pretty strait forward example to crash. I am using a global buffer for the webserver. Passing a pointer to processConnection(char *buff, int *bufflen); does not allow to pass a number directly.

When running the code, len will decrease with each request until it becomes too short or even negative, and then it will stop working.

#include <SPI.h>
#include <Ethernet.h>
#include <WebServer.h>

byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };
IPAddress ip(169,254,123,123);
WebServer server("", 80);

int len=64;
char buf[64];

void req(WebServer &server, WebServer::ConnectionType type, char* query, bool complete){
  if(!complete){
    server.httpFail();
    server.print("incomplete, len=");
    server.print(len);
    return;
  }

  if (type == WebServer::HEAD) {
    server.httpSuccess();
    return;
  }

  server.println(query);
  server.print(" len=");
  server.print(len);
}

void setup() {
  Serial.begin(115200);  
  Ethernet.begin(mac,ip);
  server.setDefaultCommand(&req);
  server.begin();
  Serial.println("UP");
}

void loop() {
  server.processConnection(buf, &len);
}

It probably should be passed by reference. The reason to pass in a modifiable length value is as stated in the comment:

// On return, length contains the amount of space left in request. If it's
// less than 0, the URL was longer than the buffer, and part of it had to
// be discarded.

Your use of len as a global variable is weird. I would expect that len would be on the stack and reinitialized before each call to processConnection. The example that uses the explicit form of processConnection does that (see https://github.com/sirleech/Webduino/blob/master/examples/Web_Authentication/Web_Authentication.ino)