Skip to content

Commit

Permalink
Fix Basic_IFS sample and TemplateStream (#2324)
Browse files Browse the repository at this point in the history
* Use per-arch hardware configs, and reduce ESP32 app. partition size

* Fix bug in ReadWriteStream::copyFrom()

* Fix TemplateStream

Tested against large (1MB) JSON listing generated from templates, issues arising.

Need to check for remaining plain data before continuing with next tag search
Move check for missing end tag after `evaluate`

* Add test for fragmented read of variable values

Verified that debug statements #1, #3 and #4 are triggered by the test.
Use smaller read buffers for testing to stress code more
mikee47 authored and slaff committed Sep 27, 2021
1 parent 4adf8c0 commit beaeb1e
Showing 7 changed files with 106 additions and 21 deletions.
4 changes: 2 additions & 2 deletions Sming/Core/Data/Stream/ReadWriteStream.cpp
Original file line number Diff line number Diff line change
@@ -23,13 +23,13 @@ size_t ReadWriteStream::copyFrom(IDataSourceStream* source, size_t size)
size_t total = 0;
size_t count;
while(!source->isFinished()) {
count = source->readMemoryBlock(buffer, sizeof(buffer));
count = source->readMemoryBlock(buffer, bufSize);
if(count == 0) {
continue;
}
auto written = write(reinterpret_cast<uint8_t*>(buffer), count);
source->seek(written);
total += count;
total += written;
if(written != count) {
break;
}
41 changes: 24 additions & 17 deletions Sming/Core/Data/Stream/TemplateStream.cpp
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@ String TemplateStream::evaluate(char*& expr)
{
auto end = strchr(expr, '}');
if(end == nullptr) {
expr += strlen(expr);
return nullptr;
}

@@ -30,7 +31,7 @@ String TemplateStream::getValue(const char* name)
if(!s && getValueCallback) {
s = getValueCallback(name);
}
debug_d("TemplateStream: value '%s' %sfound: \"%s\"", name, s ? "" : "NOT ", s.c_str());
debug_d("[TMPL] value '%s' %sfound: \"%s\"", name, s ? "" : "NOT ", s.c_str());
return s;
}

@@ -52,9 +53,16 @@ uint16_t TemplateStream::readMemoryBlock(char* data, int bufSize)
return sendValue();
}

if(valueWaitSize != 0) {
debug_d("[TMPL] #1");
size_t res = std::min(uint16_t(bufSize), valueWaitSize);
return stream->readMemoryBlock(data, res);
}

const size_t tagDelimiterLength = 1 + doubleBraces;

if(size_t(bufSize) <= TEMPLATE_MAX_VAR_NAME_LEN + (2 * tagDelimiterLength)) {
debug_d("[TMPL] #2");
return 0;
}

@@ -68,12 +76,23 @@ uint16_t TemplateStream::readMemoryBlock(char* data, int bufSize)
if(datalen != 0) {
data[datalen] = '\0'; // Terminate buffer to mitigate overflow risk
auto tagStart = findStartTag(data);
auto lastTagFound = tagStart;
while(tagStart != nullptr) {
lastTagFound = tagStart;

char* curPos = tagStart + tagDelimiterLength;
value = evaluate(curPos);
size_t tailpos = curPos - data;
if(tailpos >= datalen) {
debug_d("[TMPL #3]");
// Incomplete variable name, end tag not found in buffer
unsigned newlen = tagStart - data;
if(newlen + TEMPLATE_MAX_VAR_NAME_LEN > datalen) {
// Return what we have so far, unless we're at the end of the input stream
if(datalen == size_t(bufSize - 1)) {
debug_d("[TMPL #4]");
datalen = newlen;
break;
}
}
}
if(doubleBraces) {
// Double end brace isn't necessary, but if present skip it
if(*curPos == '}') {
@@ -113,18 +132,6 @@ uint16_t TemplateStream::readMemoryBlock(char* data, int bufSize)
memmove(data, start, valueWaitSize);
return valueWaitSize;
}

if(lastTagFound != nullptr) {
unsigned newlen = lastTagFound - data;
if(newlen + TEMPLATE_MAX_VAR_NAME_LEN > datalen) {
debug_d("TemplateStream: trim end to %u from %u", newlen, datalen);
// It can be a incomplete variable name - don't split it
// provided we're not at end of input stream
if(datalen == size_t(bufSize)) {
datalen = newlen;
}
}
}
}

datalen -= (start - data);
@@ -167,7 +174,7 @@ int TemplateStream::seekFrom(int offset, SeekOrigin origin)

if(valueWaitSize != 0) {
if(size_t(offset) > valueWaitSize) {
debug_e("TemplateStream: offset > valueWaitSize");
debug_e("[TMPL] offset > valueWaitSize");
return -1;
}
valueWaitSize -= offset;
9 changes: 9 additions & 0 deletions samples/Basic_IFS/basic_ifs_Esp32.hw
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"base_config": "basic_ifs",
"arch": "Esp32",
"partitions": {
"rom0": {
"size": "960K"
}
}
}
9 changes: 9 additions & 0 deletions samples/Basic_IFS/basic_ifs_Esp8266.hw
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"base_config": "basic_ifs",
"arch": "Esp8266",
"partitions": {
"rom0": {
"size": "480K"
}
}
}
9 changes: 9 additions & 0 deletions samples/Basic_IFS/basic_ifs_Host.hw
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"base_config": "basic_ifs",
"arch": "Host",
"partitions": {
"rom0": {
"size": "480K"
}
}
}
2 changes: 1 addition & 1 deletion samples/Basic_IFS/component.mk
Original file line number Diff line number Diff line change
@@ -10,5 +10,5 @@ ifeq ($(ENABLE_FLASHSTRING_IMAGE),1)
COMPONENT_CXXFLAGS += -DENABLE_FLASHSTRING_IMAGE=1
HWCONFIG := spiffs
else
HWCONFIG := basic_ifs
HWCONFIG := basic_ifs_$(SMING_ARCH)
endif
53 changes: 52 additions & 1 deletion tests/HostTests/modules/TemplateStream.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <HostTests.h>
#include <FlashString/TemplateStream.hpp>
#include <Data/Stream/MemoryDataStream.h>
#include <Data/Stream/LimitedMemoryStream.h>
#include <Data/Stream/SectionTemplate.h>

#ifdef ARCH_HOST
@@ -112,12 +113,62 @@ class TemplateStreamTest : public TestGroup

check(tmpl, Resource::ut_template1_out1_rst);
}

auto addChar = [](String& s, char c, size_t count) {
auto len = s.length();
s.setLength(len + count);
memset(&s[len], c, count);
};

TEST_CASE("Fragmented read of variable [TMPL #1, #3, #4]")
{
constexpr size_t TEMPLATE_BUFFER_SIZE{100};
String input;
addChar(input, 'a', TEMPLATE_BUFFER_SIZE - 4);
input += _F("{varname}");
addChar(input, 'a', TEMPLATE_BUFFER_SIZE);
auto source = new LimitedMemoryStream(input.begin(), input.length(), input.length(), false);
TemplateStream tmpl(source);
PSTR_ARRAY(someValue, "Some value or other");
tmpl.setVar(F("varname"), someValue);

size_t outlen{0};
char output[TEMPLATE_BUFFER_SIZE * 3]{};
while(!tmpl.isFinished()) {
auto ptr = output + outlen;
size_t read1 = tmpl.readMemoryBlock(ptr, TEMPLATE_BUFFER_SIZE);
char tmp[read1];
memcpy(tmp, ptr, read1);
size_t read = tmpl.readMemoryBlock(ptr, TEMPLATE_BUFFER_SIZE);
CHECK_EQ(read, read1);
CHECK(memcmp(tmp, ptr, read) == 0);
if(read > 10) {
read -= 5;
}
tmpl.seek(read);
outlen += read;
ptr += read;
}

String expected;
addChar(expected, 'a', TEMPLATE_BUFFER_SIZE - 4);
expected += someValue;
addChar(expected, 'a', TEMPLATE_BUFFER_SIZE);

if(!expected.equals(output, outlen)) {
m_nputs(output, outlen);
m_puts("\r\n");
m_nputs(expected.c_str(), expected.length());
m_puts("\r\n");
}
REQUIRE(expected.equals(output, outlen));
}
}

private:
void check(TemplateStream& tmpl, const FlashString& ref)
{
static constexpr size_t bufSize{512};
constexpr size_t bufSize{256};
char buf1[bufSize];
char buf2[bufSize];
FSTR::Stream refStream(ref);

0 comments on commit beaeb1e

Please sign in to comment.