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
  • Loading branch information
mikee47 authored May 1, 2021
1 parent fcb9186 commit b879d29
Show file tree
Hide file tree
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
Expand Up @@ -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;
}
Expand Down
41 changes: 24 additions & 17 deletions Sming/Core/Data/Stream/TemplateStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ String TemplateStream::evaluate(char*& expr)
{
auto end = strchr(expr, '}');
if(end == nullptr) {
expr += strlen(expr);
return nullptr;
}

Expand All @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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 == '}') {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
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
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit b879d29

Please sign in to comment.