bugGPSD - Bugs: bug #46802, AIVDM to CSV is broken in some...

 
 

bug #46802: AIVDM to CSV is broken in some weird cases

Submitted by:  Fred Wright <fhgwright>
Submitted on:  Sun 03 Jan 2016 10:38:25 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: Eric S. Raymond <esr>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Sun 03 Jan 2016 10:49:12 PM UTC, comment #1:

P.S.:

There are three additional uses of gpsd_hexdump in the CSV output, for types 17, 25, and 26, but those all seem to populate the bidata field unconditionally (and type 17 doesn't even have a "structured" flag), so I left them alone.

Clearly the right fix would be to implement the missing CSV formatting cases, which would render the fallback code unreachable, but it could become reachable again if new cases were implemented in the parser and not in the CSV formatter, so having a well-behaved fallback seems like a good idea.

Fred Wright <fhgwright>
Project Member
Sun 03 Jan 2016 10:38:25 PM UTC, original submission:

While trying the regression tests on a MacBook (PowerPC), I ran across some failures in the "Testing AIVDM decoding w/ CSV format" test. The failing type/dac/fid cases were as follows:

6,1,14
6,1,18
6,1,30
6,1,32
8,200,10

The proximate cause is endian dependence in the CSV output, but the real problem turns out to be that the code is broken on all platforms, but only fails on big-endian platforms because the incorrect-but-presumed-correct output was captured on a little-endian processor.

The problem arises when the type 6 or type 8 case is supported by the AIVDM parser but not by the CSV formatter. When the parser doesn't support the case, it falls back to capturing the raw binary bitdata. When the formatter doesn't support the case, it falls back to dumping the raw binary bitdata. But in the offending cases, the raw data was never correctly captured, and is instead an overlaid view of the structured data, which is incorrect (as raw data) in all cases as well as being endian-sensitive due to the presence of multibyte quantities in host byte order.

The only way to make the CSV fallback work as intended would be to change the struct ais_t type6 and type8 substructure definitions to move the bitdata field out of the union and populate it unconditionally. But this would increase the memory footprint and add to the core code for the sole purpose of providing more useful output in these cases of missing CSV formatting support, which doesn't seem very justifiable.

The alternative is simply not to report the unavailable raw bitdata at all in these cases, but instead to report something indicating that the raw data is unknown. I've implemented a version of this where the formatting remains the same (including the bit count) but the hex data is replaced by "x" characters (which seemed safer than using "?", given the absence of the latter in any existing cases.

After implementing the code change and before updating the test data, I saw one additional miscompare that I'd not seen on the PPC. This was for the 6,1,12 case, where the existing test data is too incomplete to populate the endian-sensitive fields in the decoded form, but which would have failed with a better version of the test case.

After updating the test data as well as the code, the test passes on both x86_64 and PPC.

I'm attaching a patch here.

Fred Wright <fhgwright>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #35910:  patch-aivdm-csv.diff added by fhgwright (4KiB - application/octet-stream - Patches for gpsdecode.c and test/sample.aivdm.chk)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by esr (Updated the item)
  • -unavailable- added by fhgwright (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 07 Sep 2017 04:35:19 AM UTCesrOpen/ClosedOpen=>Closed
    Mon 04 Jan 2016 08:04:27 PM UTCesrStatusNone=>Fixed
      Assigned toNone=>esr
    Sun 03 Jan 2016 10:38:25 PM UTCfhgwrightAttached File-=>Added patch-aivdm-csv.diff, #35910

    Back to the top


    Powered by Savane 3.1-cleanup1