Bug: Padding zero to WAV file metadata causes crash

[eyuan-creare]

Issue

When a metadata chunk ends on an odd byte index, I pad the chunk with a zero, which causes the Tympan to crash. Commenting this out allows the program to run as expected.

Tympan_Library/src/SDWriter.cpp

Lines 361 to 380 in ef195b2

	// --- INFO Chunk --- If Info tag specified, build a LIST.. INFO chunk and append to WAV header.
	if ( !infoKeyVal.empty() && (listInfoLoc==List_Info_Location::Before_Data) ) {
	// Calculate size of LIST chunk
	listChunk.S.chunkLenBytes = 4; // Add 4 bytes for "INFO"
	// Add up all the info tag names and strings
	for (auto &keyVal:infoKeyVal) {
	//If string is odd length, pad with 0
	if ( (keyVal.second).size()%2!=0 ){
	(keyVal.second).push_back('\0');
	}
	// Add length of Key ID (4), Key Len (4) and len of string
	listChunk.S.chunkLenBytes += 8 + (keyVal.second).size();
	}
	// update fmt chunk size
	riffChunk.S.chunkLenBytes += listChunk.S.chunkLenBytes;
	} // else no info tag, so pass

Error

Code was executing from address 0x2FE2C
14:26:10.019 ->   CFSR: 82
14:26:10.019 ->   (DACCVIOL) Data Access Violation
14:26:10.019 ->   (MMARVALID) Accessed Address: 0x0 (nullptr)
14:26:10.019 ->     Check code at 0x2FE2C - very likely a bug!
14:26:10.019 ->     Run "addr2line -e mysketch.ino.elf 0x2FE2C" for filename & line number.`

Details

• keyVal is an element of type std::map<enum class, std::string>
• I access the second element of the item, which is a std::string. keyVal.second
• Then push_back('\0') which increases the size by 1. Note that std:string manages its size independently of null terminations (but .c_str() does stop at the first NULL.
• After iterating through the entire array, I update the length of listChunk.S.chunkLenBytes += 8 + (keyVal.second).size();

Later, I concat the WAV header buffer with these key values. I use insert with the string's size and data() pointer.

for (const auto &keyVal:infoKeyVal) {
	// Append tagname (without null terminator)
	wavHeader.insert( wavHeader.end(), InfoTagToStr(keyVal.first).data(), InfoTagToStr(keyVal.first).data() + InfoTagToStr(keyVal.first).size() );

Overall, I don't see an issue with the code. @chipaudette @cab-creare-com Do you see something?

[eyuan-creare]

Note that I pad in several other places in this module.

[eyuan-creare]

Here's example code that verified this works. I tried this under the highest optimization level as well: "Smallest Code with LTO".

#include <map>
#include <string>
std::map<std::string, std::string> myMap;

void setup() {
  Serial.begin(9600);
  myMap["fruit"] = "apples";
  myMap["meat"] = "turkey";
}

void loop() {
  for (auto &item:myMap) {
    Serial.println( String(item.first.c_str()) + ": " + String(item.second.size()) + "");
    (item.second).push_back('\0');
    Serial.println( String(item.first.c_str()) + ": " + String(item.second.size()) + "");
  }
  delay(100);
}

This also works with item.second.resize(item.second.size()+1, '\0');, which extends the string with the specified character.

[eyuan-creare]

I retested my OpenHearing firmware with resize instead or push_back. It works robustly, which makes me think that something fishy is going on with either compiler optimization across modules, or C14's implementation of std::map.

[eyuan-creare]

@cab-creare-com The top of this issue points to where the Tympan was crashing. (Known by placing a print statement right before it). One thing to note is that OpenHearing is currently compiled as C++14, but the metadata library is using std::string_view, which was introduced in C++17.

Tympan_Library/src/WavHeaderFmt.h

Lines 326 to 327 in bf63016

	Info_Tags StrToTagId(std::string_view tagStr);
	std::string_view InfoTagToStr(Info_Tags tagName);

https://en.cppreference.com/w/cpp/string/basic_string_view.html