bugGPSD - Bugs: bug #46804, JSON satellite view parsing is...

 
 

bug #46804: JSON satellite view parsing is somewhat broken

Submitter:  Fred Wright <fhgwright>
Submitted:  Mon 04 Jan 2016 04:12:17 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  esr
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Mon 04 Jan 2016 04:12:17 AM UTC, original submission:  

While trying the regression tests on a MacBook (PowerPC), I ran across some failures in the JSON unit test.  Although this is ostensibly an endian issue, it turns out that the code for parsing satellite view data is actually incorrect for all processors, albeit more so for big-endian processors.

The problem is that the three "integer" fields in struct satellite_t are defined as shorts, but parsed as ints by the JSON parser.  On a big-endian processor, this causes the values to be misaddressed and hence have incorrect values, but even on a little-endian processor this is incorrect since it's storing four-byte values into two-byte fields.  The unit tests don't catch this aspect, since the fields are favorably ordered such that the clobbered fields are clobbered before being written pseudo-correctly.

I was able to demonstrate the "buffer overflow" misbehavior by modifying the test data for the last satellite to provide the fields in the reverse order from their order in the structure.

The simple fix for this would be just to change the shorts to ints in the definition of struct satellite_t.  On most processors, this doesn't even cost any memory, since the presence of the double forces eight-bye alignment, so the padded structure is 24 bytes regardless of whether the three fields in question are shorts or ints.  However, there might be some processors with less strict alignment requirements where using shorts would actually be helpful.

With the existing layout, the only possible fix is to add support for shorts to the JSON parser, and adjust the satellite-view parsing accordingly.  The attached patch does that, as well as adding u_short support for completeness (though it's not currently used).  It also provides the aforementioned change in the test data, in keeping with the philosophy of "create a test for what just failed, so it doesn't happen again".

Note that using shorts for these fields would be more effective if the "used" field were also reduced to a short, instead of inheriting "int" from "bool".  That would shrink the structure to 16 bytes.  It could be further reduced to 12 bytes by using a float instead of a double for the "ss" field (and even a float is gross overkill for this purpose).  This could all be more significant when MAXCHANNELS needs to be increased (again) to accommodate the deployment of the newer GNSSes.

Fred Wright <fhgwright>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #35911:  patch-json-sats.diff added by fhgwright (4KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-09-07 esr Open/ClosedOpen Closed
    2016-01-04 esr StatusNone Fixed
        Assigned toNone esr
    2016-01-04 fhgwright Attached File- Added patch-json-sats.diff, #35911

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code