1275331 - REST API methods return integers as strings

Status: NEW

(Bugzilla :: WebService, defect, P1)

Description

[Dylan Hardison [:dylan] (he/him)]

Some API methods can return strings when they shouldn't.

To illustrate and understand what's going on, you have to know that perl scalars can have both a string part and a number part.


~(master)% perl -MDevel::Peek -e 'my $iv = 42; Dump $iv'
SV = IV(0x7fe3740059a0) at 0x7fe3740059a8
  REFCNT = 1
  FLAGS = (PADMY,IOK,pIOK)
  IV = 42

$v is a scalar value with an integer part set to 42.

perl -MDevel::Peek -e 'my $pv = "42"; Dump $pv'
SV = PV(0x7fde29003678) at 0x7fde290059a8
  REFCNT = 1
  FLAGS = (PADMY,POK,pPOK)
  PV = 0x7fde28f00300 "42"\0
  CUR = 2
  LEN = 16

So far, so good. All the JSON modules will turn the first into a number and the second into a string.
However, if we use $ic in a string anywhere...

perl -MDevel::Peek -e 'my $iv = 42; my $pv = "$iv"; Dump $iv'
SV = PVIV(0x7fe2b0822e10) at 0x7fe2b08059a8
  REFCNT = 1
  FLAGS = (PADMY,IOK,POK,pIOK,pPOK)
  IV = 42
  PV = 0x7fe2b0701580 "42"\0
  CUR = 2
  LEN = 16

JSON::PP, JSON::XS will always encode that as a string.

I suspect this is one of the reasons many people prefer Cpanel::JSON::XS these days. From the Cpanel::JSON::XS documentation:

"fixed encode of numbers for dual-vars. Different string representations are preserved, but numbers with temporary strings which represent the same number are here treated as numbers, not strings. Cpanel::JSON::XS is a bit slower, but preserves numeric types better."

for module in JSON::PP JSON::XS Cpanel::JSON::XS; do
    echo -n using $module ': '
    perl -M$module -E 'my $iv = 30; my $pv = "$iv"; say encode_json({age => $iv})'
done

using JSON::PP : {"age":"30"}
using JSON::XS : {"age":"30"}
using Cpanel::JSON::XS : {"age":30}

It has been strongly recommended that Bugzilla drop JSON/JSON::XS in favor of JSON::MaybeXS. If we do that this bug is solved. Otherwise we must closely guard that data from web apis is never used in a string context before being sent to the client. Cloninng each result structure is *not* an option for performance reasons (I did a little benchmarking, it's very expensive.)

soo... which way do we want to take this?

Comment 1

[Frédéric Buclin]

Several questions:

1) which clients are affected by this problem? If it's not a problem, then there is nothing to fix.
2) what's wrong with JSON::XS (related to our REST API)? Only this issue?
3) Cpanel::JSON::XS is said to be a bit slower (than JSON::XS?). How much slower?

Comment 2

[Dylan Hardison [:dylan] (he/him)]

(In reply to Frédéric Buclin from comment #1)
> Several questions:
> 
> 1) which clients are affected by this problem? If it's not a problem, then
> there is nothing to fix.
Many JS clients and python clients can be broken silently, because of how they treat a + b when a or b are strings.

> 2) what's wrong with JSON::XS (related to our REST API)? Only this issue?
It has issues with not having a public issue tracker.

> 3) Cpanel::JSON::XS is said to be a bit slower (than JSON::XS?). How much
> slower?

In my testing, less than 1ms.
However we have a new way forward (see next comment)

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Frédéric pointed out that my $age = 30.0; my $str = "$age"; results in an $age that is still rendered as an integer, even by JSON::PP. The reason for this appears to be in how those modules handle scalars whose number part is NV rather than IV.

Examine: perl -MDevel::Peek -E 'my $i = 30.0; my $s = "$i"; Dump($i)'

We can exploit this by doing the following for "integer" conversion:

my $val = 30;
my $int = (int($val) . ".0") + 1

This value will never[1] be turned into a string by any of the JSON::* modules.

Comment 4

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #3)
> Frédéric pointed out that my $age = 30.0; my $str = "$age"; results in an
> $age that is still rendered as an integer, even by JSON::PP. The reason for
> this appears to be in how those modules handle scalars whose number part is
> NV rather than IV.
> 
> Examine: perl -MDevel::Peek -E 'my $i = 30.0; my $s = "$i"; Dump($i)'
> 
> We can exploit this by doing the following for "integer" conversion:
> 
> my $val = 30;
> my $int = (int($val) . ".0") + 1
> 
> This value will never[1] be turned into a string by any of the JSON::*
> modules.

I was not able to get this to work locally using JSON::XS. I vote we change to Cpanel::JSON::XS as a dependency.

dkl