Opened 7 years ago

Closed 7 years ago

#2992 closed defect (fixed)

JSON to number conversion does not check errno on C++

Reported by: fujiwara Owned by: fujiwara
Priority: medium Milestone: Sprint-20130625
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

In function src/lib/cc/data.cc:fromStringstreamNumber(),
it cannot carry LONG_MAX and -LONG_MAX value because they are treated as error.
If strtol return value is LONG_MAX or -LONG_MAX, need to check errno variable. If errno is ERANGE or EINVAL, it is a error.

Subtickets

Attachments (1)

test1.diff (865 bytes) - added by fujiwara 7 years ago.
corner case test of integer data conversion (64bit only)

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by muks

  • Milestone changed from New Tasks to Next-Sprint-Proposed

Changed 7 years ago by fujiwara

corner case test of integer data conversion (64bit only)

comment:2 Changed 7 years ago by fujiwara

I added test1.diff which contains corner case test (+LONG_MAX, -LONG_MAX) of integer conversion. However, LONG_MAX is different between 64bit architecture and 32bit architecture.
Do you consider that BIND 10 does not run on systems which do not have int64_t/uint64_t ?

comment:3 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 2

comment:4 Changed 7 years ago by fujiwara

Current code does not catch floating underflow.
I made trac2992 branch and pushed my fix.

[2992] Added errno check to fromStringstreamNumber()

Now support LONG_MAX, LONG_MIN and floating underflow detection

[2992] Added integer conversion tests (LONG_MAX, -LONG_MAX, LONG_MIN)

Added floating underflow test

Does it make sense ?

comment:5 Changed 7 years ago by muks

Fujiwara-san, if you think this branch is complete and can be reviewed to fix this bug, put this bug to review.

comment:6 Changed 7 years ago by fujiwara

Thanks, I think this branch is OK if we continue to use strtol/strtod and existing JSON parser.

comment:7 Changed 7 years ago by fujiwara

  • Owner set to UnAssigned
  • Status changed from new to reviewing

comment:8 Changed 7 years ago by muks

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

comment:9 Changed 7 years ago by pselkirk

  • Owner changed from UnAssigned to pselkirk

comment:10 Changed 7 years ago by pselkirk

  • Owner changed from pselkirk to fujiwara

Fujiwara-san, this looks fine to me. Please merge.

comment:11 Changed 7 years ago by fujiwara

Thanks for reviewing. I will merge it.

comment:12 Changed 7 years ago by fujiwara

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.