workaround bootloader-shared var init bug

[diablodale]

Workaround for luxonis/depthai-bootloader-shared#4

• Manually init class success and char array so that both are in a valid and known state as
  compared to them being in a random and/or non-null terminated state
• After a related PR in depthai-bootloader-shared is merged, these workarounds should be removed

[themarpe]

@diablodale Thanks

Could you rather check for the return value of the receive/parse response functions and remove the setting the fields beforehand. It'll still be correct and with the shared PR merged it'll then also add the additional protection against further cases

[themarpe]

Response::FlashComplete result;
if(receiveResponse(result)) {
    return {result.success, result.errorMsg};
} else {
    return {false, ""};
}

[diablodale]

Several question. Please guide me.

Have plans changed? You told me here luxonis/depthai-bootloader-shared#4 (comment) that I should manually init the variables in depthai-core because the classes fix would not be merged because it requires a rebuild/distrib of the firmware. So the 6 code locations are errant until the bootloader-shared PR is merged and the depthai-core changes its firmware archive to use that new firmware.

It is unclear the full meaning of the success since it isn't doc'd and I can't see firmware code. I would need guidance on the different uses of .success for at least the lines (in PR line numbers) 544, 629, 682, 716. Even in cases where I can remove the success check, I still need to set errorMsg[0]=0 because in the fail/false cases, the return still creates a std::string within a tuple based on the decayed errorMsg pointer. (This is why i want to change the class to std::string for safety, but I that requires firmware changes I can't see).

[themarpe]

Only in case where receive/parse response function returns true, can we assume that the result is "well formed" (its treated as POD type). That's why I think the suggested change is better.

(also suggested this approach instead in: luxonis/depthai-bootloader-shared#4 (comment))

In lines 544, 629, 682 case, we'd want to rather do (along the lines of):

bool success = false;
std::string errorMsg = "";
do {
    std::vector<uint8_t> data;
    if(!receiveResponseData(data)) return {false, "Couldn't receive bootloader response"};

    Response::FlashStatusUpdate update;
    Response::FlashComplete result;
    if(parseResponse(data, update)) {
        // if progress callback is set
        if(progressCb != nullptr) {
            progressCb(update.progress);
        }
    } else if(parseResponse(data, result)) {
        success = result.success;
        errorMsg = std::string(result.errorMsg);
        break;
    } else {
        // Unknown response, shouldn't happen
        return {false, "Unknown response from bootloader while flashing"};
    }
} while(true);

and in 716:

// Get response
Response::GetBootloaderConfig resp;
if(receiveResponse(resp) && resp.success) {
    // Read back bootloader config (1 packet max)
    auto bsonConfig = stream->read();
    // Parse from BSON
    return nlohmann::json::from_bson(bsonConfig);
} else {
    return {};
}

[diablodale]

We might agree to disagree on the approach. That's ok. 👍🙂 The approach you suggest has too many concerns for me to put my name on.

I intend to use this PR's approach in my build. I force pushed a new update that includes a few changes based on your info above. If this PR doesn't work for you...completely ok. You can deny this PR and your team can take the approach you prefer. ✌

As friendly assist, here are those concerns of mine 🙂:

• be sure your team fixes all 6 locations (1 of which is in a comment) affected by this bug.
• your two example codes above remain high risk. The code hopes that no part of funcs, serialization, xlink, firmware, device, and roundtrip makes a mistake and doesn't write uint32 to the .success or a null terminated string to .errorMsg. Optimization could easily skip writing a single null for no error/string -or- devs making again the same mistake they already made in this code. A buffer bug like that leads to security buffer overrun, maybe crash, or have 64 random characters in the created std::string
• This series of bugs are on error-case paths. It is likely they are not being heavily exercised in testing. Therefore, I lean towards caution, strong coding guidelines, etc. This is that class of bugs caused by failing to init struct/class to a known state. We are human, we make mistakes and forget. Devs will forget to write special-case extra code.
• Special-case handling to avoid using un-initialized structs is extra code+testing, extra source of bugs, extra memory, and extra cpu cycles. A higher resource use + higher risk = doesn't make sense for my builds

[themarpe]

We can do both, I'm fine with having those set to known values beforehand also, with the requested checks being there as well. (and we can remove those being set explicitly afterwards the shared changes get merged)

So current state of the PR LGTM

Running "The Matrix"...

[themarpe]

Response::FlashComplete result;
if(receiveResponse(result)) {
    return {result.success, result.errorMsg};
} else {
    return {false, ""};
}

[themarpe]

"The Matrix" has spoken - some minor style issues regarding ifs & comments

[diablodale]

Yes, I will always put a space after if. I need a tool to fix these for me. Vscode is occassionaly not correcting the format; seems to be when I don't specifically press the semicolon key, or when pasting. I found more vscode settings to better cover those cases. Hopefully will get yet better formatting moving forward.

i have force pushed