Skip to content

My first change - #1409

Open
yazdan-iot wants to merge 6 commits into
FujiNetWIFI:masterfrom
yazdan-iot:my-first-change
Open

My first change#1409
yazdan-iot wants to merge 6 commits into
FujiNetWIFI:masterfrom
yazdan-iot:my-first-change

Conversation

@yazdan-iot

Copy link
Copy Markdown
Contributor

Fix: 7 Bugs in lib/http/

Summary

This PR fixes 7 bugs found in the lib/http/ folder. All bugs are small and safe to merge.


Bug 1: Buffer not null-terminated

Files:

  • lib/http/fnHttpClient.cpp line 825
  • lib/http/mgHttpClient.cpp line 1113

Problem:
strncpy() does not add \0 at the end when the source string is longer than the buffer. Any code reading this buffer will read garbage memory.

Example:

buffer_len = 10
Header value = "very long header value" (22 chars)
Result: "very long " + garbage...

Fix:

// Before
return strncpy(buffer, vi->second.c_str(), buffer_len);

// After
strncpy(buffer, vi->second.c_str(), buffer_len - 1);
buffer[buffer_len - 1] = '\0';
return buffer;

Bug 2: url_encode() crashes on low memory

File: lib/http/httpService.cpp line 68

Problem:
malloc() can return NULL when memory is low. The code does not check and writes to NULL pointer, causing crash.

Fix:

char *buf = (char *)malloc(strlen(str) * 3 + 1);
if (buf == NULL)
    return NULL;

Bug 3: url_decode() crashes on low memory

File: lib/http/httpService.cpp line 87

Problem:
Same as Bug 2. malloc() without NULL check.

Fix:

char *buf = (char *)malloc(strlen(str) + 1);
if (buf == NULL)
    return NULL;

Bug 4: Off-by-one in query parsing

File: lib/http/httpService.cpp line 344

Problem:
Using path_end - 1 instead of path_end cuts one character from the path.

Example:

URI: /mount?hostslot=1
Before: path = "/moun"  (wrong)
After:  path = "/mount" (correct)

Fix:

// Before
results->path += results->full_uri.substr(0, path_end - 1);

// After
results->path += results->full_uri.substr(0, path_end);

Bug 5: COPY() always sends MOVE request

File: lib/http/mgHttpClient.cpp line 987

Problem:
The COPY() function ignores the move parameter. It always sets HTTP_MOVE, even when copying.

Fix:

// Before
_method = HTTP_MOVE;

// After
_method = move ? HTTP_MOVE : HTTP_COPY;

Bug 6: strncat() can overflow buffer

File: lib/http/httpServiceParser.cpp line 655

Problem:
strncat() receives the total buffer size instead of remaining space. Can write past the buffer.

Fix:

// Before
strncat(result, "\">", MAX_PRINTER_LIST_BUFFER);

// After
strncat(result, "\">", MAX_PRINTER_LIST_BUFFER - strlen(result) - 1);

Bug 7: Wrong unsigned comparison

File: lib/http/fnHttpClient.cpp line 300

Problem:
size() returns unsigned. Comparing with <= 0 is meaningless because unsigned can never be negative.

Fix:

// Before
if (client->_stored_headers.size() <= 0)

// After
if (client->_stored_headers.size() == 0)

Files Changed

File Bugs Fixed
lib/http/fnHttpClient.cpp Bug 1, Bug 7
lib/http/mgHttpClient.cpp Bug 1, Bug 5
lib/http/httpService.cpp Bug 2, Bug 3, Bug 4
lib/http/httpServiceParser.cpp Bug 6

Testing

All fixes are small and focused:

  • Bug 1: Prevents buffer read overflow
  • Bug 2-3: Prevents NULL pointer crash
  • Bug 4: Fixes path parsing
  • Bug 5: Fixes HTTP method selection
  • Bug 6: Prevents buffer write overflow
  • Bug 7: Code clarity fix

Related

All bugs are in the HTTP library (lib/http/). They affect:

  • HTTP client header reading
  • URL encoding/decoding
  • Query string parsing
  • WebDAV COPY/MOVE operations
  • Printer list HTML generation

abolfazl33369 and others added 4 commits June 29, 2026 19:04
If malloc() fails to allocate the response buffer, _buffer remains nullptr. The HTTP event handler later calls memcpy() into this buffer without checking, which would cause a crash on data reception. This adds a null check in the constructor (with a debug log) and a guard in the HTTP_EVENT_ON_DATA handler to safely drop data instead of crashing if the buffer wasn't allocated.
Comment thread lib/http/fnHttpClient.cpp
Comment on lines 814 to +827

char *fnHttpClient::get_header(int index, char *buffer, int buffer_len)
{
if (index < 0 || index > (_stored_headers.size() - 1))
return nullptr;

if (buffer == nullptr)
return nullptr;

auto vi = _stored_headers.begin();
std::advance(vi, index);
return strncpy(buffer, vi->second.c_str(), buffer_len);
strncpy(buffer, vi->second.c_str(), buffer_len - 1);
buffer[buffer_len - 1] = '\0';
return buffer;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier to replace strncpy() with strlcpy() which will guarantee it's null terminated.

Comment thread lib/http/fnHttpClient.cpp
Comment on lines 26 to 29
if (_buffer == nullptr)
{
Debug_printf("fnHttpClient::fnHttpClient() failed to allocate %d byte buffer\r\n", DEFAULT_HTTP_BUF_SIZE);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this fix is already in master. You probably need to rebase this branch on master. First go to your fork and go to your master branch and click the Sync fork button. Then from your cloned repo:

git checkout master
git pull
git checkout my-first-change
git rebase master
git push --force-with-lease

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks

Comment thread lib/http/mgHttpClient.cpp Outdated
Comment on lines +1113 to +1115
strncpy(buffer, vi->second.c_str(), buffer_len - 1);
buffer[buffer_len - 1] = '\0';
return buffer;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, just use strlcpy()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants