Skip to content

Commit

Permalink
Fix UBSAN bugs (#2845)
Browse files Browse the repository at this point in the history
This PR fixes some potential/actual bugs identified by the undefined behaviour sanitizer.
Several of these became apparent with `-Wextra`, mainly through 'unused parameter' warnings, of which there are many.

**String::replace: Passing nullptr to memcpy is undefined behaviour**

**axtls sha1: Undefined behaviour**

axtls-8266/crypto/sha1.c:124:42: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior axtls-8266/crypto/sha1.c:124:42

Because: implicit conversion of uint8_t to int

**Don't force queueCallback with no parameter**

Sanitizer reports undefined behaviour passing parameter to function which doesn't expect one.
Works fine in practice but safer to add a lamba for dealing with it.

**Stream::parseFloat() not handling timeout**

**String(unsigned char, ...) ignoring width, pad parameters**

**char could be signed, check for `> 127` would never succeed**

**HostTests datetime check should use parameter**

**Fix unimplemented host `RtcClass::setRtcNanoseconds()`**

**Potential array bounds violation in esp-open-lwip dhcp.c**

Check for index in range should be first.

```
lwip/core/dhcp.c: In function 'dhcp_recv':
lwip/core/dhcp.c:133:69: warning: array subscript 10 is above array bounds of 'u8_t[10]' {aka 'unsigned char[10]'} [-Warray-bounds]
  133 | #define dhcp_option_given(dhcp, idx)          (dhcp_rx_options_given[idx] != 0)
      |                                                ~~~~~~~~~~~~~~~~~~~~~^~~~~
lwip/core/dhcp.c:592:9: note: in expansion of macro 'dhcp_option_given'
  592 |   while(dhcp_option_given(dhcp, DHCP_OPTION_IDX_DNS_SERVER + n) && (n < DNS_MAX_SERVERS)) {
      |         ^~~~~~~~~~~~~~~~~
lwip/core/dhcp.c:131:7: note: while referencing 'dhcp_rx_options_given'
  131 | u8_t  dhcp_rx_options_given[DHCP_OPTION_IDX_MAX];
      |       ^~~~~~~~~~~~~~~~~~~~~
```
  • Loading branch information
mikee47 authored Jun 29, 2024
1 parent 6570fd3 commit 3db6495
Show file tree
Hide file tree
Showing 9 changed files with 48 additions and 15 deletions.
11 changes: 10 additions & 1 deletion Sming/Arch/Esp8266/Components/esp-open-lwip/esp-open-lwip.patch
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ index 77ef471..0e4936a 100644
static const char mem_debug_file[] ICACHE_RODATA_ATTR = __FILE__;
#endif
diff --git a/lwip/core/dhcp.c b/lwip/core/dhcp.c
index 342543e..92ad15b 100644
index 342543e..cd7d0c3 100644
--- a/lwip/core/dhcp.c
+++ b/lwip/core/dhcp.c
@@ -137,6 +137,9 @@ u8_t dhcp_rx_options_given[DHCP_OPTION_IDX_MAX];
Expand All @@ -503,6 +503,15 @@ index 342543e..92ad15b 100644

/* DHCP client state machine functions */
static err_t dhcp_discover(struct netif *netif);
@@ -586,7 +589,7 @@ dhcp_handle_ack(struct netif *netif)
#if LWIP_DNS
/* DNS servers */
n = 0;
- while(dhcp_option_given(dhcp, DHCP_OPTION_IDX_DNS_SERVER + n) && (n < DNS_MAX_SERVERS)) {
+ while((n < DNS_MAX_SERVERS) && dhcp_option_given(dhcp, DHCP_OPTION_IDX_DNS_SERVER + n)) {
ip_addr_t dns_addr;
ip4_addr_set_u32(&dns_addr, htonl(dhcp_get_option_value(dhcp, DHCP_OPTION_IDX_DNS_SERVER + n)));
dns_setserver(n, &dns_addr);
diff --git a/lwip/core/ipv4/ip_addr.c b/lwip/core/ipv4/ip_addr.c
index 81db807..dd6964e 100644
--- a/lwip/core/ipv4/ip_addr.c
Expand Down
2 changes: 1 addition & 1 deletion Sming/Arch/Host/Platform/RTC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ uint32_t RtcClass::getRtcSeconds()

bool RtcClass::setRtcNanoseconds(uint64_t nanoseconds)
{
return false;
return setRtcSeconds(nanoseconds / 1'000'000'000ULL);
}

bool RtcClass::setRtcSeconds(uint32_t seconds)
Expand Down
13 changes: 13 additions & 0 deletions Sming/Components/axtls-8266/axtls-8266.patch
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,19 @@ index 53509d0..25c568d 100644
}
#endif

diff --git a/crypto/sha1.c b/crypto/sha1.c
index 1082733..097302a 100644
--- a/crypto/sha1.c
+++ b/crypto/sha1.c
@@ -121,7 +121,7 @@ static void SHA1ProcessMessageBlock(SHA1_CTX *ctx)
*/
for (t = 0; t < 16; t++)
{
- W[t] = ctx->Message_Block[t * 4] << 24;
+ W[t] = (uint32_t)ctx->Message_Block[t * 4] << 24;
W[t] |= ctx->Message_Block[t * 4 + 1] << 16;
W[t] |= ctx->Message_Block[t * 4 + 2] << 8;
W[t] |= ctx->Message_Block[t * 4 + 3];
diff --git a/replacements/time.c b/replacements/time.c
index 4972119..1447711 100644
--- a/replacements/time.c
Expand Down
2 changes: 1 addition & 1 deletion Sming/Core/Data/Format/Json.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ void Json::escape(String& value) const
debug_w("Invalid UTF8: %s", value.c_str());
for(unsigned i = 0; i < value.length(); ++i) {
char& c = value[i];
if(c < 0x20 || c > 127)
if(c < 0x20 || uint8_t(c) > 127)
c = '_';
}
}
Expand Down
6 changes: 6 additions & 0 deletions Sming/Platform/System.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ bool SystemClass::queueCallback(TaskCallback callback, void* param)
reinterpret_cast<os_param_t>(param));
}

bool SystemClass::queueCallback(InterruptCallback callback)
{
return queueCallback([](void* param) { reinterpret_cast<InterruptCallback>(param)(); },
reinterpret_cast<void*>(callback));
}

bool SystemClass::queueCallback(TaskDelegate callback)
{
if(!callback) {
Expand Down
5 changes: 1 addition & 4 deletions Sming/Platform/System.h
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,7 @@ class SystemClass
/**
* @brief Queue a deferred callback with no callback parameter
*/
__forceinline static bool IRAM_ATTR queueCallback(InterruptCallback callback)
{
return queueCallback(reinterpret_cast<TaskCallback>(callback));
}
static bool IRAM_ATTR queueCallback(InterruptCallback callback);

/**
* @brief Queue a deferred Delegate callback
Expand Down
3 changes: 1 addition & 2 deletions Sming/Wiring/Stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,9 @@ float Stream::parseFloat(char skipChar)
bool isNegative = false;
bool isFraction = false;
long value = 0;
char c;
float fraction = 1.0;

c = peekNextDigit();
int c = peekNextDigit();
// ignore non numeric leading characters
if(c < 0) {
return 0; // timeout
Expand Down
19 changes: 14 additions & 5 deletions Sming/Wiring/WString.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ String::String(char c) : String()
String::String(unsigned char value, unsigned char base, unsigned char width, char pad) : String()
{
char buf[8 + 8 * sizeof(value)];
ultoa(value, buf, base);
ultoa_wp(value, buf, base, width, pad);
*this = buf;
}

Expand Down Expand Up @@ -811,13 +811,18 @@ bool String::replace(const char* find_buf, size_t find_len, const char* replace_
if(len == 0 || find_len == 0) {
return true;
}
if(replace_buf == nullptr) {
replace_len = 0;
}
int diff = replace_len - find_len;
char* readFrom = buf;
const char* end = buf + len;
char* foundAt;
if(diff == 0) {
while((foundAt = (char*)memmem(readFrom, end - readFrom, find_buf, find_len)) != nullptr) {
memcpy(foundAt, replace_buf, replace_len);
if(replace_len) {
memcpy(foundAt, replace_buf, replace_len);
}
readFrom = foundAt + replace_len;
}
} else if(diff < 0) {
Expand All @@ -826,8 +831,10 @@ bool String::replace(const char* find_buf, size_t find_len, const char* replace_
size_t n = foundAt - readFrom;
memmove(writeTo, readFrom, n);
writeTo += n;
memcpy(writeTo, replace_buf, replace_len);
writeTo += replace_len;
if(replace_len) {
memcpy(writeTo, replace_buf, replace_len);
writeTo += replace_len;
}
readFrom = foundAt + find_len;
len += diff;
}
Expand All @@ -851,7 +858,9 @@ bool String::replace(const char* find_buf, size_t find_len, const char* replace_
readFrom = buf + index + find_len;
memmove(readFrom + diff, readFrom, len - (readFrom - buf));
len += diff;
memcpy(buf + index, replace_buf, replace_len);
if(replace_len) {
memcpy(buf + index, replace_buf, replace_len);
}
index--;
}
setlen(len);
Expand Down
2 changes: 1 addition & 1 deletion tests/HostTests/modules/DateTime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class DateTimeTest : public TestGroup

void checkHttpDates(const FSTR::Array<TestDate>& dates)
{
for(auto date : VALID_HTTP_DATE) {
for(auto date : dates) {
DateTime dt;
String s(*date.stringToParse);
Serial << s << endl;
Expand Down

0 comments on commit 3db6495

Please sign in to comment.