diff options
author | Fred Wright <fw@fwright.net> | 2016-01-04 13:58:48 -0500 |
---|---|---|
committer | Eric S. Raymond <esr@thyrsus.com> | 2016-01-04 13:58:48 -0500 |
commit | 227605a14b89e2cb9d85e148db7f99fe825f3fe2 (patch) | |
tree | 1cfbf9304e32c360216542bd73877d5344bed4de /json.c | |
parent | b6807735feafa583329b2ce4bc0d48287a64b9e8 (diff) | |
download | gpsd-227605a14b89e2cb9d85e148db7f99fe825f3fe2.tar.gz |
Address Savannah bug #46804: JSON satellite view parsing is somewhat broken.
Fred Wright <fhgwright> writes:
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.
Diffstat (limited to 'json.c')
-rw-r--r-- | json.c | 43 |
1 files changed, 43 insertions, 0 deletions
@@ -128,6 +128,12 @@ static char *json_target_address(const struct json_attr_t *cursor, case t_uinteger: targetaddr = (char *)&cursor->addr.uinteger[offset]; break; + case t_short: + targetaddr = (char *)&cursor->addr.shortint[offset]; + break; + case t_ushort: + targetaddr = (char *)&cursor->addr.ushortint[offset]; + break; case t_time: case t_real: targetaddr = (char *)&cursor->addr.real[offset]; @@ -197,6 +203,13 @@ static int json_internal_read_object(const char *cp, case t_uinteger: memcpy(lptr, &cursor->dflt.uinteger, sizeof(unsigned int)); break; + case t_short: + memcpy(lptr, &cursor->dflt.shortint, sizeof(short)); + break; + case t_ushort: + memcpy(lptr, &cursor->dflt.ushortint, + sizeof(unsigned short)); + break; case t_time: case t_real: memcpy(lptr, &cursor->dflt.real, sizeof(double)); @@ -478,6 +491,18 @@ static int json_internal_read_object(const char *cp, memcpy(lptr, &tmp, sizeof(unsigned int)); } break; + case t_short: + { + short tmp = atoi(valbuf); + memcpy(lptr, &tmp, sizeof(short)); + } + break; + case t_ushort: + { + unsigned short tmp = (unsigned int)atoi(valbuf); + memcpy(lptr, &tmp, sizeof(unsigned short)); + } + break; case t_time: { double tmp = iso8601_to_unix(valbuf); @@ -645,6 +670,24 @@ int json_read_array(const char *cp, const struct json_array_t *arr, cp = ep; break; #endif /* JSON_MINIMAL */ + case t_short: +#ifndef JSON_MINIMAL + arr->arr.shorts.store[offset] = (short)strtol(cp, &ep, 0); + if (ep == cp) + return JSON_ERR_BADNUM; + else + cp = ep; + break; +#endif /* JSON_MINIMAL */ + case t_ushort: +#ifndef JSON_MINIMAL + arr->arr.ushorts.store[offset] = (unsigned short)strtoul(cp, &ep, 0); + if (ep == cp) + return JSON_ERR_BADNUM; + else + cp = ep; + break; +#endif /* JSON_MINIMAL */ case t_time: #ifndef JSON_MINIMAL if (*cp != '"') |