Hi, There are still two open issues with the static analyzer at [0].
Both are at ecc-mod(), where the hi value can be used uninitialized on two occasions; ecc-mod.c:104 and ecc-mod.c:110. The issue can be addressed by initializing to zero, but although it looks reasonable, I'm not sure if that's the expectation of the algorithm.
Addressing that issue would allow for successful CI runs.
regards, Nikos
[0]. Build Job: https://gitlab.com/gnutls/nettle/-/jobs/31862567
Report: https://gitlab.com/gnutls/nettle/-/jobs/31862567/artifacts/browse/scan-build...
Nikos Mavrogiannopoulos nmav@redhat.com writes:
There are still two open issues with the static analyzer at [0].
A few weeks ago (prior to fixing the "dead increment" issue), I installed the clang static analyzer locally, but I didn't get the same issues reported. I got a few issues inside mini-gmp which looked like false positives (iirc, complaints were about some division function doing something bad in case qp was a NULL pointer but q wasn't, clang not realizing that the function logic makes qp == NULL if and only if q == NULL).
Both are at ecc-mod(), where the hi value can be used uninitialized on two occasions; ecc-mod.c:104 and ecc-mod.c:110. The issue can be addressed by initializing to zero, but although it looks reasonable, I'm not sure if that's the expectation of the algorithm.
I try to avoid initializing things just to avoid warnings. I'm reasonably confident there are no uninitialized values used at run time, since (1) all conditions involve sizes which are pretty static, not depending on numerical values of the bignums, and (2) I've seen no complaints about uninitialized values when running the testsuite under valgrind.
If we changed the code to initialize the variable was initialized to some dummy value, valgrind would no longer be able to spot such problems in this code.
Actually, I've also seen warnings about this code from gcc, for the use at line 104, but only when compiling for sparc (and on a machine that I no longer use).
Addressing that issue would allow for successful CI runs.
I agree that's desirable.
This code needs some careful analysis, to see under what conditions hi might be used uninitialized, and in case there's some valid inputs for which this could happen fix that, and if not, back up our assumptions with asserts (which I hope the static analyzer will understand).
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
This code needs some careful analysis, to see under what conditions hi might be used uninitialized, and in case there's some valid inputs for which this could happen fix that, and if not, back up our assumptions with asserts (which I hope the static analyzer will understand).
It turns out analysis isn't that subtle. The thing is, the static analyzer thinks that the loop condition (rn >= 2*mn - bn) can be initially false.
Now, rn is initialized to 2*mn, so this could be false only if the subtraction ounderflows. Which is doesn't do, since valid range for bn is 0 < bn < mn.
Question is what's the best way to make thhat clear to compilers and analyzers.
Loops could be rewritten as do {} while (0) loops. Or we could add more asserts, maybe it's sufficient to replace the somewhat weak
assert (sn > 0);
with
assert (bn < mn);
Since sn is size_t (unsigned), the former only checks that mn != bn.
This code is a bit hairy as is, so we should try to not make it even more complex.
Regards, /Niels
On Wed, Sep 13, 2017 at 8:09 AM, Niels Möller nisse@lysator.liu.se wrote:
nisse@lysator.liu.se (Niels Möller) writes:
This code needs some careful analysis, to see under what conditions hi might be used uninitialized, and in case there's some valid inputs for which this could happen fix that, and if not, back up our assumptions with asserts (which I hope the static analyzer will understand).
It turns out analysis isn't that subtle. The thing is, the static analyzer thinks that the loop condition (rn >= 2*mn - bn) can be initially false.
Now, rn is initialized to 2*mn, so this could be false only if the subtraction ounderflows. Which is doesn't do, since valid range for bn is 0 < bn < mn.
Question is what's the best way to make thhat clear to compilers and analyzers.
Loops could be rewritten as do {} while (0) loops. Or we could add more asserts, maybe it's sufficient to replace the somewhat weak
assert (sn > 0);
with
assert (bn < mn);
Since sn is size_t (unsigned), the former only checks that mn != bn.
I tried with few asserts() but couldn't eliminate this error. We may be hitting an analyzer bug, or missing something there.
I've even added:
if (rn < 2 * mn - bn) <---NEW abort(); <--- NEW while (rn >= 2 * mn - bn)
prior to the while above, but the analyzer still thinks that the while path can be skipped.
The only way I could eliminate the error was through the attached patch which is ugly.
Maybe we should put this whole function in #ifndef __clang_analyzer__
block to allow a smooth CI run.
regards, Nikos
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
I tried with few asserts() but couldn't eliminate this error. We may be hitting an analyzer bug, or missing something there.
In theory, it could also happen that rn = 2*mn overflows, potentially making rn == 0.
if (rn < 2 * mn - bn) <---NEW abort(); <--- NEW while (rn >= 2 * mn - bn)
prior to the while above, but the analyzer still thinks that the while path can be skipped.
I'd tend to say that's an analyzer bug. (Regarding analyzer and asserts, it might be useful to have it say which asserts it believes can't happen, but when it thinks an assert might trigger, it should still assume that it doesn't in following code.
The only way I could eliminate the error was through the attached patch which is ugly.
Rewriting using do {} while loop would make some sense, to make it clear both to the analyzer and to humans that loops are intended to run at least once. Would you like to try that out, to see if it gets ugly or not? Both loops should be changed in the same way, for consistency, even if only one matters at the moment.
BTW, I logged the input sizes when running the testsuite (which uses this for more moduli than the non-test code), and the possible cases are
normalized, mn = 4, sn = 2 unnormalized, mn = 3, sn = 1 unnormalized, mn = 4, sn = 1 unnormalized, mn = 4, sn = 3 unnormalized, mn = 6, sn = 3 unnormalized, mn = 9, sn = 4 unnormalized, mn = 9, sn = 8
for a 64-bit build. In a 32-bit bild, I get
normalized, mn = 7, sn = 4 normalized, mn = 8, sn = 1 unnormalized, mn = 12, sn = 6 unnormalized, mn = 12, sn = 7 unnormalized, mn = 17, sn = 16 unnormalized, mn = 17, sn = 8 unnormalized, mn = 6, sn = 3 unnormalized, mn = 7, sn = 3 unnormalized, mn = 8, sn = 7
Maybe we should put this whole function in #ifndef __clang_analyzer__
block to allow a smooth CI run.
If we find no better way, that would work to enable conntinuous static analysis of the rest
Regards, /Niels
On Wed, 2017-09-13 at 10:43 +0200, Niels Möller wrote:
The only way I could eliminate the error was through the attached patch which is ugly.
Rewriting using do {} while loop would make some sense, to make it clear both to the analyzer and to humans that loops are intended to run at least once. Would you like to try that out, to see if it gets ugly or not?
I'm a little overwhelmed at the moment to start with that. If there is nothing after few months, I may return to it.
regards, Nikos
Nikos Mavrogiannopoulos nmav@redhat.com writes:
Rewriting using do {} while loop would make some sense, to make it clear both to the analyzer and to humans that loops are intended to run at least once. Would you like to try that out, to see if it gets ugly or not?
I'm a little overwhelmed at the moment to start with that. If there is nothing after few months, I may return to it.
Ok. I'll try to get to it reasonably soon.
Regards, /Niels
nisse@lysator.liu.se (Niels Möller) writes:
Rewriting using do {} while loop would make some sense, to make it clear both to the analyzer and to humans that loops are intended to run at least once.
With the below patch, the static analyzer stops complaining when I run it locally. I still get two reports for --enable-mini-gmp, though, for the tq adjustments in mpz_div_qr, which I think are false positives. I'm running scan-build from debian's clang-3.9 package.
Question is if the patch is more or less ugly than adding an
#ifdef __clang_analyzer__ hi = 0; #endif
at the top of the function. Personally, I find the do {... } while style a bit unusual and disturbing here.
Regards, /Niels
diff --git a/ecc-mod.c b/ecc-mod.c index 5fee4c6..7a58462 100644 --- a/ecc-mod.c +++ b/ecc-mod.c @@ -51,7 +51,7 @@ ecc_mod (const struct ecc_modulo *m, mp_limb_t *rp) mp_size_t i; unsigned shift;
- assert (sn > 0); + assert (bn < mn);
/* FIXME: Could use mpn_addmul_2. */ /* Eliminate sn limbs at a time */ @@ -59,7 +59,7 @@ ecc_mod (const struct ecc_modulo *m, mp_limb_t *rp) { /* Multiply sn + 1 limbs at a time, so we get a mn+1 limb product. Then we can absorb the carry in the high limb */ - while (rn > 2 * mn - bn) + do { rn -= sn;
@@ -68,11 +68,13 @@ ecc_mod (const struct ecc_modulo *m, mp_limb_t *rp) rp[rn-1] = rp[rn+sn-1] + mpn_add_n (rp + rn - sn - 1, rp + rn - sn - 1, rp + rn - 1, sn); } + while (rn > 2 * mn - bn); + goto final_limbs; } else { - while (rn >= 2 * mn - bn) + do { rn -= sn;
@@ -83,6 +85,7 @@ ecc_mod (const struct ecc_modulo *m, mp_limb_t *rp) hi = cnd_add_n (hi, rp + rn - mn, m->B, mn); assert (hi == 0); } + while (rn >= 2 * mn - bn); }
if (rn > mn)
On Wed, Sep 13, 2017 at 9:57 PM, Niels Möller nisse@lysator.liu.se wrote:
nisse@lysator.liu.se (Niels Möller) writes:
Rewriting using do {} while loop would make some sense, to make it clear both to the analyzer and to humans that loops are intended to run at least once.
With the below patch, the static analyzer stops complaining when I run it locally. I still get two reports for --enable-mini-gmp, though, for the tq adjustments in mpz_div_qr, which I think are false positives. I'm running scan-build from debian's clang-3.9 package.
Question is if the patch is more or less ugly than adding an
#ifdef __clang_analyzer__ hi = 0; #endif
Seems good I think. Maybe with a comment on why this was added.
regards, Nikos
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
On Wed, Sep 13, 2017 at 9:57 PM, Niels Möller nisse@lysator.liu.se wrote:
Question is if the patch is more or less ugly than adding an
#ifdef __clang_analyzer__ hi = 0; #endif
Seems good I think. Maybe with a comment on why this was added.
I committed a variant of this ifdef thing this morning. But the gnutls mirror and the ci machinery seems to not have picked it up yet.
Regards, /Niels
On Thu, 2017-09-14 at 23:28 +0200, Niels Möller wrote:
Nikos Mavrogiannopoulos n.mavrogiannopoulos@gmail.com writes:
On Wed, Sep 13, 2017 at 9:57 PM, Niels Möller <nisse@lysator.liu.se
wrote: Question is if the patch is more or less ugly than adding an
#ifdef __clang_analyzer__ hi = 0; #endif
Seems good I think. Maybe with a comment on why this was added.
I committed a variant of this ifdef thing this morning. But the gnutls mirror and the ci machinery seems to not have picked it up yet.
There was an update of gitlab.com, and something may have been broken. I've checked the settings of the project on mirroring and says that 'update is scheduled'. I'll give it a day before opening a bug.
regards, Nikos
Nikos Mavrogiannopoulos nmav@redhat.com writes:
On Thu, 2017-09-14 at 23:28 +0200, Niels Möller wrote:
I committed a variant of this ifdef thing this morning. But the gnutls mirror and the ci machinery seems to not have picked it up yet.
There was an update of gitlab.com, and something may have been broken. I've checked the settings of the project on mirroring and says that 'update is scheduled'. I'll give it a day before opening a bug.
It seems the commits pushed to my master repo September 14 are still not picked up by the mirror.
Regards, /Niels
On Wed, 2017-09-20 at 17:55 +0200, Niels Möller wrote:
Nikos Mavrogiannopoulos nmav@redhat.com writes:
On Thu, 2017-09-14 at 23:28 +0200, Niels Möller wrote:
I committed a variant of this ifdef thing this morning. But the gnutls mirror and the ci machinery seems to not have picked it up yet.
There was an update of gitlab.com, and something may have been broken. I've checked the settings of the project on mirroring and says that 'update is scheduled'. I'll give it a day before opening a bug.
It seems the commits pushed to my master repo September 14 are still not picked up by the mirror.
Thanks for the reminder. There is already an open ticket for that which I commented on: https://gitlab.com/gitlab-com/support-forum/issues/2420
regards, Nikos
On Wed, 2017-09-20 at 18:32 +0200, Nikos Mavrogiannopoulos wrote:
On Wed, 2017-09-20 at 17:55 +0200, Niels Möller wrote:
Nikos Mavrogiannopoulos nmav@redhat.com writes:
On Thu, 2017-09-14 at 23:28 +0200, Niels Möller wrote:
I committed a variant of this ifdef thing this morning. But the gnutls mirror and the ci machinery seems to not have picked it up yet.
There was an update of gitlab.com, and something may have been broken. I've checked the settings of the project on mirroring and says that 'update is scheduled'. I'll give it a day before opening a bug.
It seems the commits pushed to my master repo September 14 are still not picked up by the mirror.
Thanks for the reminder. There is already an open ticket for that which I commented on: https://gitlab.com/gitlab-com/support-forum/issues/2420
Should be working now. I see that master-updates and curve448 pass.
regards, Nikos
nettle-bugs@lists.lysator.liu.se
-
Nikos Mavrogiannopoulos -
Nikos Mavrogiannopoulos -
Nikos Mavrogiannopoulos -
nisse＠lysator.liu.se