Opened 7 years ago

Closed 7 years ago

#2571 closed defect (fixed)

type error in data.cc (breaks Raspberry Pi builds)

Reported by: jinmei Owned by: shane
Priority: medium Milestone: Sprint-20130122
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 2
Total Hours: 0 Internal?: no

Description

In lib/cc/data.cc, this is not correct:

    char c = in.peek();
    while (char_in(c, chars) && c != EOF) {

The type of return value of peek() is int, and c != EOF is bogus
(always-true expression) if char is unsigned. Overall variables c
in this file should be defined as int.

This seems to break ARM build.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by shane

  • Owner changed from UnAssigned to shane
  • Status changed from new to accepted

comment:2 Changed 7 years ago by shane

Okay, since this broke the build on my Raspberry Pi, I made the fixes to get this to work.

The main issue here is that data.cc was treating character-by-character input having type char, when in fact it has type int. There is a FAQ covering the use of EOF which goes into great detail:

http://faq.cprogramming.com/cgi-bin/smartfaq.cgi?id=1043284351&answer=1048865140

I've adjusted the appropriate types in data.cc, as well as changing a few conversions in exceptions.

There was also an instance in perfdhcp that I changed for similar-though-not-identical reasons.

It turns out that because of the type conversion issue, the JSON-from-string was actually broken before, in the case where the character '\xFF' appeared in the string. I've added a test for that case, which breaks under the old code but works after the fixes.

This brings up a slightly larger issue, which is that in principle JSON strings are Unicode, defaulting to UTF-8:

http://tools.ietf.org/html/rfc4627#section-3

This may not apply to our situation, as it would mean that we need to use ICU or something like that and convert to Unicode throughout:

http://site.icu-project.org/

I think just being 8-bit clean is good enough at this point, but if you think otherwise please feel free to open a ticket.

comment:3 Changed 7 years ago by shane

  • Defect Severity changed from N/A to Medium
  • Milestone changed from New Tasks to Next-Sprint-Proposed
  • Owner changed from shane to UnAssigned
  • Status changed from accepted to reviewing

comment:4 Changed 7 years ago by shane

  • Summary changed from type error in data.cc to type error in data.cc (breaks Raspberry Pi builds)

comment:5 Changed 7 years ago by vorner

I don't know for which purpose we would need to treat it as unicode. I mean,
unless we do fancy things like taking substrings of 5 graphemes (not 5 bytes
because of storing them, but 5 bits of image to show) or make somethings
uppercase, there's nothing much which could break. If we just take the data,
store and send them somewhere else (log, library, other process) or concatenate
them, we are good without knowing which charset it is, just having the
representation.

comment:6 Changed 7 years ago by shane

Oh, in case it is not clear, there is a branch (trac2571) with the fixes in it.

comment:7 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130122

comment:8 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:9 Changed 7 years ago by jinmei

It basically looks okay with a few minor notes:

  • I added some piggy-back cleanup and one small optimization. Please check them.
  • I believe we need a changelog entry in the "build" (or bug) category.
  • I'd like to have some comments about the intent of this added test:
    +    sv.push_back("\"\xFF\"");
    
  • Commit log messages should have "[2571]".

comment:10 Changed 7 years ago by jinmei

  • Owner changed from jinmei to shane

comment:11 Changed 7 years ago by shane

  • Add Hours to Ticket changed from 0 to 2
  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged into master.

Note: See TracTickets for help on using tickets.