Mantis Bug Tracker

View Issue Details Jump to Notes ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0000631EMStudioFreeEMS Pluginpublic2012-07-28 18:322013-01-29 13:27
ReporterFred 
Assigned Tomalcom2073 
PrioritynormalSeverityminorReproducibilityalways
StatusassignedResolutionfixed 
PlatformAllOSAllOS VersionAll
Product Version 
Target Version0.0.1Fixed in Version 
Summary0000631: Reject interrogation of firmware with invalid location ID meta data
DescriptionReject interrogation of firmware with location IDs that have different lengths to defined location IDs in the meta data file or protocol/interface definition.

Undefined locations may exist and be assigned to a hex editor.

Defined locations should contain sufficient meta data to determine their length and render their contents in a human readable way.
Additional InformationSpecial types such as 2D and 3D tables should be verified on load and rejected if not of the defined-by-interface size. (64 and 1024, respectively).

Such tables should additionally verify that the table is not broken or corrupt:

IE, for 3D, the axis values should be sequential and the numbers of axis values given in the first four bytes should be allowed and combine to not overflow 1024.

2D only needs to verify the axis values on load.

This should likely present the user with the opportunity to fix it in future, but for now it can either reject the firmware outright or display as hex.
TagsNo tags attached.
Issue TypeBug
Attached Filespng file icon EmstudioLyingToMeAboutWhatItFoundAndWhen.png [^] (29,724 bytes) 2013-01-29 13:23

- Relationships

-  Notes
User avatar (0001804)
malcom2073 (manager)
2012-08-03 12:53

Check for size in a1f10a375cd219d7f3c646511b33afebdfc27e2b
Need to add checks for axis sequentiallness, though I'm not sure that should be the concern of the tuner?
User avatar (0001805)
Fred (administrator)
2012-08-03 21:06

When two pieces of software have a contract, it is of both of their responsibilities equally. Thus I should verify that when you send me data, it's good, and you should verify that when I send you data, it's good. It's not really part of this ticket, but you should be doing it. Trust no one, especially not me, and also especially not your users. That leaves no one that isn't especially untrustworthy. You should reject my shit (likely change of protocol) as I reject yours (likely bug or incompatible version). This is just good practice. Because it's at boot up only, and you're running on a 1ghz+ processor, it's trivial to be thorough.
User avatar (0001806)
malcom2073 (manager)
2012-08-04 16:28

Done in e9ee4b26c07e62dc1cddab23c1897a388144c18e with a caveat:

Since I only keep one copy of ram/flash data in memory, I am unable to check the axis values until the table actually gets loaded on a double click in EmsInfoView. If the axis are invalid, it throws an error message to the user that their firmware is broken, and does not load the table. Let me know if you want it to instead load as hex, or what. There is no way for meta-data to screw this up, so if an axis is indeed in the wrong order, it is firmware fault.
User avatar (0001807)
malcom2073 (manager)
2012-08-04 16:29

Also, I am unable to test this since I can't have broken firmware. You should test this with a custom firmware build to have invalid axis's.
User avatar (0001820)
Fred (administrator)
2012-08-06 16:11

"so if an axis is indeed in the wrong order, it is firmware fault" or who ever bypassed the checks and sent invalid data via parent location ;-)

I will have a play when i get a chance.

Other comments from IRC that you may lose in the haze of Ben and I discussing OLV:

(16:37:38) Fred: mal|lappy: "size" : "1024"
(16:37:46) Fred: from a main table
(16:37:59) Fred: that's protocol knowledge right now
(16:38:11) Fred: ditto 2d
(16:38:18) Fred: "size" : "64"
(16:39:14) Fred: "size":"2"
(16:39:14) Fred: for the core vars fields, it shouldn't have a size, it should have a type, each type has a size
(16:39:40) Fred: good that you don't have an overall size, though, sum of parts = whole
(16:39:41) Fred: good.
(16:41:09) Fred: "arraysize" : "24"
(16:41:09) Fred: arraysize should be in elements not bytes
(16:47:21) Fred: mal|lappy: i see what you've done re loading anyway, and pop up on click. sizes should be part of interrogation "Does what I'm getting match the file in key/all places?" If yes, load, if no, fail and produce error report.
User avatar (0001821)
Fred (administrator)
2012-08-06 16:19

Reopened and assigned back until robust/complete.
User avatar (0001830)
malcom2073 (manager)
2012-08-07 01:04

Check out f60ee8b01641cecfb0d89adc1e05f0d1bb8d2f35

You need to let me know what action you want me to take upon finding a table or axis that is invalid.
User avatar (0002578)
Fred (administrator)
2013-01-29 13:27

Attached file shows no sign of failure and just looks stalled/frozen. Turn the last one red for each section, use your best judgement.

Additionally, it's dishonest. I put the bad data in the json for ID 0 and it shows OK for ID 0 and 6, when it wasn't OK for 0 and should never have gotten to 6.

{ "tables" : {
 "VE Table" : {
  "locationid" : "0x0000",
  "type" : "3D",
  "xtitle" : "rpm",
  "xcalc" : [{"type" : "div", "value" : "2.0"}],
  "xdp" : "0",
  "xhighlight" : "RPM",
  "ytitle" : "load",
  "ycalc" : [{"type" : "div", "value" : "100.0"}],
  "ydp" : "0",
  "yhighlight" : "MAP",
  "ztitle" : "VE",
  "zcalc" : [{"type" : "div","value" : "512.0"}],
  "zdp" : "1",
  "size" : "1023"
 },

Change "In progress" to "Pending" as in progress is wrong. We've had this discussion before, and that to me/anyone means "on the wire" not "queued waiting to be sent"

I guess I'd leave the 5% bar as it is, but change it to red, and make the failed ID be red, and the Pending ones blank or failed or grey, but no longer pending.


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker