Crash in YAxisRendererRadarChart

[owenbirdsall]

Crashes on line 137: axis.decimals = Int(ceil(-log10(interval)))

When interval is 0.
In my case this is happening when rawInterval is 0.05, interval then gets set to 0, and -log10(0) results in NaN which causes ceil to crash.

[liuxuan30]

why rawInterval is 0.05 leads to interval 0?

[owenbirdsall]

That's what I believe I saw in the debugger when I trapped the exception... I will try again to verify the exact value for you. Any case would it not make sense to guard against zero here?

…

On 14 Apr 2017, at 06:39, Xuan ***@***.***> wrote: why rawInterval is 0.05 leads to interval 0? ― You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[owenbirdsall]

Sorry, it was range that was 0.05 not rawInterval. So when you have a range of 0.05, rawInterval becomes 0.0083* and in turn interval becomes 0. Which results in a crash.

[owenbirdsall]

Presumably changing line 135 to:
if interval > 0 && interval < 1
Would fix the issue?

[liuxuan30]

I need you to give more details on how this leads to crash, and what's the test data, so we can reproduce with ChartsDemo. There has been a long time for this kind of issue.

[owenbirdsall]

There is a vulnerability because of this piece of code on line 137 of YAxisRendererRadarChart:
axis.decimals = Int(ceil(-log10(interval)))

If you add the following code var x = Int(ceil(-log10(0))) to your demo it will crash.

There is no guard against interval being zero, and if it is zero it will cause a crash. As far as I can see there is nothing preventing data being used in a radar chart that can result in an interval of zero.

In my case I had a range of 0.05, which caused rawInterval to be 0.0083* and hence interval is zero... hence the crash.

A simple guard against zero whenever you use this Int(ceil(-log10(interval))) piece of code should fix your issues.

[liuxuan30]

Why interval in your case is Int 0? It's supposed to be Double value.

I tried with 0.05 range like below, it still does not crash.

(lldb) po range
0.050000000000000003

(lldb) po rawInterval
0.01

(lldb) po interval
0.01

(lldb) po Int(ceil(-log10(interval)))
2

The interval is a float decimal value, not as Int 0.

[owenbirdsall]

It's not an integer, it's a double - but a double can still be zero (i.e. 0.0)
...and -log10(0.0) results in a NaN
...and ceil(NaN) causes a crash

I don't see anything in the code which guarantees that interval is > 0.0

[owenbirdsall]

Here are the values I have in my debugger at the point of my crash:

[owenbirdsall]

From what I can see interval is >0.0 until it reaches the following block of code:

interval = floor(10 * intervalMagnitude)

This floor operation can cause interval to go to 0.0

[liuxuan30]

Well range=0.05 will not crash, but range=0.3785 will. Looks like we missed some corner cases
As rawInterval is 0.06308, the interval is 0.06, and intervalMagnitude=0.01, intervalSigDigit=6

Now the problem is intervalSigDigit=6 will trigger:

        if intervalSigDigit > 5
        {
            // Use one order of magnitude higher, to avoid intervals like 0.9 or
            // 90
            interval = floor(10 * intervalMagnitude)
        }

and unfortunately, floor(10 * 0.01) is 0.0 indeed

I can't say this is a bug or design flaw, but right now you can use

interval = floor(10 * intervalMagnitude) == 0.0 ? interval : floor(10 * intervalMagnitude)

to fix. I will add a PR for this later.

Note, interval can be replaced with intervalMagnitude * 5 (e.g. to get 0.05 rather than 0.06, 0.08) or whatever interval looks good to you.

[owenbirdsall]

Thanks for looking into this, much appreciated.