Coverity Scan API usage error (SWAPPED_ARGUMENTS)

[fhunleth]

We use Coverity Scan to analyze our source code, and it flagged an error in monocypher.c. This very much looks like a false positive to me, but in an abundance of caution, I figured that I'd post it here:

*** CID 355285:  API usage errors  (SWAPPED_ARGUMENTS)
/src/3rdparty/monocypher-3.0.0/src/monocypher.c: 1970 in ge_scalarmult_base()
1964                 fe_ccopy(t2, comb_T2[j], select);
1965             }
1966     
1967             fe_neg(n2, t2);
1968             fe_cswap(t2, n2, high);
1969             fe_cswap(yp, ym, high);
>>>     CID 355285:  API usage errors  (SWAPPED_ARGUMENTS)
>>>     The positions of arguments in the call to "ge_madd" do not match the ordering of the parameters:
* "ym" is passed to "yp"
* "yp" is passed to "ym"
1970             ge_madd(p, p, ym, yp, n2, a, t2); // reuse t2 as temporary
1971         }
1972         WIPE_CTX(&dbl);
1973         WIPE_BUFFER(yp);  WIPE_BUFFER(t2);  WIPE_BUFFER(a);
1974         WIPE_BUFFER(ym);  WIPE_BUFFER(n2);
1975         WIPE_BUFFER(s_scalar);

[LoupVaillant]

Hi, thanks for the heads up, I didn't know about Coverity Scan.

What it flagged here is not an error (I swapped those arguments on purpose), but it is a code smell. The code for the upcoming 3.1.0 version (release candidates are already out) is much cleaner. The tool was right to flag this ugly piece of code.

Now why did I do it anyway?

We start with a point, determined by the following parameters:

yp = y + x
ym = y - x
t2 = y × x × constant

Where (x, y) are the Edwards coordinates of our point. We use the weird coordinates to speed up point addition. What matters is that to negate an Edwards point, you just need to negate the x coordinate.

-(x, y) = (-x, y)

Negating the weird coordinates is a tiny bit more complicated:

-(yp, ym, t) = -(y+x, y-x, yxt) = (y-x, y+x, -t) = (ym, yp, -t)

Now to get the argument in the "correct" order, I could apply this patch:

       - ge_madd(p, p, ym, yp, n2, a, t2);
       + ge_msub(p, p, yp, ym, t2, a, t2);

That reveals my true intention: I want to subtract the conditionally negated point. Problem was, ge_msub() was first implemented in terms of ge_madd(): it first negated the incoming point, then called ge_madd(). This incurred a small, but measurable overhead. Calling ge_madd() directly avoided that overhead.

That overhead is now removed (at the cost of a few duplicated lines of code), and the current code uses ge_msub(). No more code smell.

[fhunleth]

Great! Thanks for the quick response and very thorough explanation.