Fix HalModuleParser.parsePrefix() when parsing valid compressed binaries #41

avtolstoy · 2021-05-25T23:01:30Z

Current version of binary-version-reader may provide invalid module prefix for valid compressed modules due to hardware platform-specific heuristics.

This causes errors in Device Service and disables compression and module combination when performing OTAs:

RangeError: Invalid size of the module data\
 at combineModules (/usr/src/app/node_modules/binary-version-reader/lib/moduleEncoding.js:405:10)\
 at runMicrotasks (<anonymous>)\
 at processTicksAndRejections (internal/process/task_queues.js:93:5)\
 at async Flash._combineModuleBinaries (/usr/src/app/lib/behaviors/Flash.js:475:18)\
 at async Flash._preprocessModuleBinary (/usr/src/app/lib/behaviors/Flash.js:394:20)\
 at async Flash.flash (/usr/src/app/lib/behaviors/Flash.js:130:12)"

The following binary after compression will not be correctly parsed:
tracker-system-part1@3.1.0-alpha.2.bin

Compressed:
compressed_tracker-system-part1@3.1.0-alpha.2.bin

 CRC is ok (35df8d60)
 Compiled for unknown platform (ID: 19388)
 This is an unknown module number 182 at version 8993
 It depends on undefined number 20 at version 62570

This should instead be:

 CRC is ok (35df8d60)
 Compiled for tracker
 This is a system module number 1 at version 3101
 It depends on a bootloader number 0 at version 1006
 It also depends on a network coprocessor (NCP) module number 0 at version 7

…refix validation

…s unknown platform, flags and function

… causes HalModuleParser.parsePrefix() to parse the module prefix at incorrect location

wraithan

Overall the code looks good, reads a lot better and I appreciate the code clean up you've done.

I'm a bit 🤔 about the strict option to _validateModulePrefix and would like to discuss before approval.

lib/HalModuleParser.js

wraithan · 2021-05-26T15:59:16Z

lib/HalModuleParser.js

+			0x184, // Gen 2
+			0xC0, // Bluez
+			0x10C, // Core
+		];


Ah this reads far better I really like this cleanup

At first I was going to suggest adding a zero offset to the list of preferred ones, but then I noticed that it's actually there, it just didn't deserve its own line in the array definition. I'd add a comment that the module header typically starts at 0 offset if it's a user part or other non-system/bootloader module.

Moved 0 to a separate line and added an additional comment.

sergeuz

What an awesome PR 👍

sergeuz · 2021-05-27T11:56:27Z

lib/HalModuleParser.js

+		const expectedSize = endAddr - startAddr + 4 /* CRC-32 */;
+		if (fileBuffer.length < expectedSize) {
+			return result;
+		}


A comment why we don't want to fail immediately if the file is larger than what is specified in the module header would be helpful here (padded/combined modules).

https://github.com/particle-iot/binary-version-reader/pull/41/files#diff-1191fb254480711d2964abf2c8ce9727c95f0e24d7a032142ff18c792b49b14bR306

sergeuz · 2021-05-27T11:58:03Z

lib/HalModuleParser.js

+		const allFlags = Object.values(ModuleInfo.Flags).reduce((a, b) => (a | b), 0);
+		if ((prefixInfo.moduleFlags & (~allFlags)) === 0) {
+			result.score++;
+		}


Non-zero version numbers (of the module itself and its dependencies) can be another 0.25 score points 🙂 Also, the fact that start/end addresses are in the internal flash (where applicable) that was checked in the old code was a good indication that we're dealing with a valid module, IMO.

Also, the fact that start/end addresses are in the internal flash (where applicable) that was checked in the old code was a good indication that we're dealing with a valid module, IMO.

I think we should be able to get away without these kinds of platform-specific checks and probably should if we can. It's also impossible to implement such checks properly for Gen 3 platforms as addresses in the internal flash are generic 0 to 1MB.

Non-zero version numbers (of the module itself and its dependencies) can be another 0.25 score points 🙂

As discussed in Slack, let's keep things as-is. Given all the other checks it's anyway highly likely that the versions will not be zero.

sergeuz · 2021-05-27T12:04:20Z

lib/HalModuleParser.js

-			(userModuleStartAddy.indexOf("20") == 0);   // stack is located in ram at 0x2000000
-
-		if (isCore) {
-			return 0x10C;


Oh yes. Thank you so much for doing this.

sergeuz · 2021-05-27T12:06:00Z

lib/HalModuleParser.js

+			0x184, // Gen 2
+			0xC0, // Bluez
+			0x10C, // Core
+		];


At first I was going to suggest adding a zero offset to the list of preferred ones, but then I noticed that it's actually there, it just didn't deserve its own line in the array definition. I'd add a comment that the module header typically starts at 0 offset if it's a user part or other non-system/bootloader module.

sergeuz · 2021-05-27T12:31:59Z

lib/HalModuleParser.js

@@ -23,9 +23,9 @@ var when = require('when');
 var pipeline = require('when/pipeline');
 var utilities = require('./utilities.js');
 var buffers = require('h5.buffers');


This module is no longer used in this file. I'd also check if we even need this dependency at all, given the changes in this PR.

You are right, no longer required. Removed the dependency.

wraithan

Thanks for the updates, looks great

…version-reader#41)

avtolstoy added the bug label May 25, 2021

avtolstoy requested review from wraithan and sergeuz May 25, 2021 23:01

avtolstoy added 3 commits May 26, 2021 07:37

HalModuleParser: removes some of the hardware-specifics from module p…

a1e4580

…refix validation

[test] HalModuleParser: adds a test with a module binary that contain…

1347367

…s unknown platform, flags and function

[test] moduleEncoding: adds problematic binary that after compression…

37764d1

… causes HalModuleParser.parsePrefix() to parse the module prefix at incorrect location

avtolstoy force-pushed the feature/ch80114/fix-halmoduleparser-parseprefix-when-parsing branch from 83f985b to 37764d1 Compare May 26, 2021 00:37

wraithan requested changes May 26, 2021

View reviewed changes

HalModuleParser: removes strict argument in _validateModulePrefix

e680fef

avtolstoy requested a review from wraithan May 27, 2021 06:29

sergeuz approved these changes May 27, 2021

View reviewed changes

avtolstoy added 2 commits May 27, 2021 20:58

minor

07809f1

removes h5.buffers dependency

807c47c

wraithan approved these changes May 27, 2021

View reviewed changes

avtolstoy merged commit 94e26d9 into master May 27, 2021

avtolstoy deleted the feature/ch80114/fix-halmoduleparser-parseprefix-when-parsing branch May 27, 2021 14:45

avtolstoy added a commit to particle-iot/particle-cli that referenced this pull request May 28, 2021

bump binary-version-reader dependency to ^1.0.1 (particle-iot/binary-…

e4a9b18

…version-reader#41)

avtolstoy mentioned this pull request May 28, 2021

[Gen 3] 256KB application binary support when flashing over DFU particle-iot/particle-cli#610

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HalModuleParser.parsePrefix() when parsing valid compressed binaries #41

Fix HalModuleParser.parsePrefix() when parsing valid compressed binaries #41

avtolstoy commented May 25, 2021

wraithan left a comment

wraithan May 26, 2021

sergeuz May 27, 2021

avtolstoy May 27, 2021

sergeuz left a comment

sergeuz May 27, 2021

avtolstoy May 27, 2021

sergeuz May 27, 2021

avtolstoy May 27, 2021

sergeuz May 27, 2021

sergeuz May 27, 2021

sergeuz May 27, 2021

avtolstoy May 27, 2021

wraithan left a comment

Fix HalModuleParser.parsePrefix() when parsing valid compressed binaries #41

Fix HalModuleParser.parsePrefix() when parsing valid compressed binaries #41

Conversation

avtolstoy commented May 25, 2021

wraithan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wraithan left a comment

Choose a reason for hiding this comment