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

 
 

bug #46804: JSON satellite view parsing is somewhat broken

Submitted by:  Fred Wright <fhgwright>
Submitted on:  Mon 04 Jan 2016 04:12:17 AM 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.

 

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>
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 #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
  • -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:10 AM UTCesrOpen/ClosedOpen=>Closed
    Mon 04 Jan 2016 07:01:58 PM UTCesrStatusNone=>Fixed
      Assigned toNone=>esr
    Mon 04 Jan 2016 04:12:17 AM UTCfhgwrightAttached File-=>Added patch-json-sats.diff, #35911

    Back to the top


    Powered by Savane 3.1-cleanup1