Fixes other rounding errors

[superbuggy]

Mirrors https://github.com/RedHatProductSecurity/cvss/pull/65/files which addresses #66. Also addresses some code issues and a bug in the main branch.

[skontar]

@pandatix, @bjedwards, I know you guys previously worked on yet-another-rounding issue, would you please look at this? We have a fix in Python library and would like to push both fixes at once.

[bjedwards]

I think this is a little more complicated than we need? I assume it's following the CVSS 3.1 rounding specifications?

But all it should need is Math.round(value*10)/10. If we want to be careful about some possible floating points, we could tweak it to Math.round((value + Number.EPSILON)*10)/10, but I can't find an edge case that doesn't work with the simpler version.

The second parseFloat(value.toFixed(2)), is just doing a Number->string->Number conversion on a number that is already the correct value.

[skontar]

Ah, wrong link, it should have been Python library. One of the examples is:

8.6 - 7.15 = 1.4499999999999993 (float) => 1.5

I guess adding a small value would also work.

[skontar]

8.6 - 7.15 + Number.EPSILON = 1.4499999999999995 

[superbuggy]

@bjedwards The function amended in the pull request will still suffer from rounding errors that are an effect of floating point misrepresentations.

8.6 - 7.15 will return 1.4499999999999993 rather than 1.45. When rounded to one decimal place, this will produce 1.4 which is not correct.

[pandatix]

I align with Ben's comment and I'll go further: we must not make the rounding more difficult than the CVSS v4.0 scoring...

The community is looking at the source code in this repository to understand what is going on under the hood, and reimplement it for other ecosystems. If the rounding is 20-lines long, what is the message ? That rounding is difficult in JS ? (yes it does)
Or will it be that CVSS v4.0 does not know how to round numbers ? Or is it very specific thus need to be reimplemented in all ecosystems ?

I advocate for a simplistic approach, as for previous iterations, with a function that rounds enough: a sum with an epsilon then basic rounding, a manual rounding with 5 decimals, ... something simple yet efficient. We are not computing a black hole radiation here, only a score from 0 to 10 with 1 decimal.

[skontar]

I agree with you on some point, but we are rounding it incorrectly. Anyone using exact arithmetic available in some languages will get correct value. Even a person with a calculator will get the correct value. But we are off in some cases, again, in a similar fashion as we did in v3.0.

We could probably make the rounding function simpler and still correct, in Python it is two lines – round to 5 places, round to 1 place.

I am fine with leaving it as is, but I got so many rounding bug reports and questions on v3.0 that I do not really want to repeat that experience (that is why I wanted to avoid floats altogether).

[skontar]

Adding references:
#66
RedHatProductSecurity/cvss#63

[pandatix]

I agree with you on some point, but we are rounding it incorrectly.

Yes, that is a sure thing 🫤
That was more a note from me on the lack of a standard for rounding in the spec. That is a good thing to do when dealing with floats, and we are now discussing due to its lack. Next iteration must contain it if uses floating numbers, else use integers !

[superbuggy]

For your consideration, please see consult the example here that illustrates the issue.

Number.EPSILON represents the smallest difference between one and a floating point number and is too small to prevent a rounding error in some cases.

While not unique to JavaScript, some of the difficulty arises from Number.prototype.toFixed having implicit rounding.

We can leverage the implicit rounding there to make less verbose code, however I find this less communicative to the reader who is not as familiar with JavaScript.

function roundToDecimalPlaces(value, decimalPlaces = 1) {
    // Step 1: Shift the decimal point by multiplying with 10^BUFFER_SIZE
    const BUFFER_SIZE = 5;
    const factor = Math.pow(10, BUFFER_SIZE);
    const reRoundFactor = Math.pow(10, decimalPlaces);

    // Step 2: Apply the ROUND_HALF_UP logic by rounding to the nearest integer
    // After shifting the decimal point
    const shiftedValue = value * factor;
    const roundedValue = Math.round(shiftedValue);

    // Step 3: Shift the decimal point back by dividing with the same factor
    const unshiftedValue = (roundedValue / factor);

    // Step 4: Re-round the value with an additional buffering step to prevent floating point rounding errors in previous step
    // eg. 8.6 - 7.15 will return 1.4499999999999993 rather than 1.45. When this misrepresented, floating-point difference is rounded to one decimal place, this will produce 1.4 which is not correct
    const reRoundedValue = Math.round(unshiftedValue * reRoundFactor) / reRoundFactor;

    // Step 5: Ensure the re-rounded has the correct number of decimal places
    return parseFloat(reRoundedValue.toFixed(decimalPlaces));
}

Since there is not a specific and inherent level of complexity to 20 lines of code, the verbosity and comments should send the message to the reader that we both care about meaningful accuracy and the ease of comprehension for the reader.

[superbuggy]

I can test out something like value => Math.round((value + 10**-6)*10)/10 if that is preferable.

[superbuggy]

That works. What about something like...

function roundToDecimalPlaces(value) {
    // Based on interval machine epsilon value for single-precision floating point decimals: https://en.wikipedia.org/wiki/Machine_epsilon
    const EPSILON = Math.pow(10, -6);
    return Math.round((value + EPSILON) * 10) / 10;
}

with some examples thrown in? @bjedwards

[bjedwards]

This likely appears to get us where we need. I'd agree with @pandatix that we don't want to make it appear that CVSS4.0 requires special rounding (although CVSSv3 did :D).

We could always introduce a library dependency...
https://mathjs.org/

But I acutally think @superbuggy 's last is the best approach.

[skontar]

I have matched this fix with RedHatProductSecurity/cvss#65 in Python implementation. We checked that both implementations give the same results. @bjedwards @pandatix , I believe this is the best option. It is not complicated, it yields correct and expected results, as well as have a readable example of why we do this. Let me ask reporter of #66 and RedHatProductSecurity/cvss#63 to double-check.

[skontar]

@Xsze, double-checked the results. Unless anyone objects soon I will merge both rounding fixes in both libraries to be consistent.