Hello,

"uninitialized random" https://curl.haxx.se/docs/adv_20161223.html
says:
"
This mistake managed to slip in because:

1. It wasn't detected by manual code reviews
"

which I would like to modify as follows:

0. The code used an old-style BREAK-CAST *)
in order to work around (not: treat!) a transition issue caused by mismatch of API signatures
(ROOT CAUSE)

1. very relevant activity did code changes *and* moved files
   (which strongly hampered detection by manual code reviews)

[...]

*) this is a bit of a stretch, since
with C language, this cast isn't old-style: there is that kind of cast only
(only C++ has "some" cast protection against the worst kinds of interface transition abuses).
In C, ways to still try to avoid destructive cast issues might be:
- (for sufficiently frequently invoked code parts) to create some helper functions which
  do the casting internally, thus in a controlled, maximally central manner
- avoid API transition mismatch issues

If this code didn't use a BREAK-CAST *), then that massively wrongly-typed
assignment would have been immediately shot down by the compiler.

*) BTW very *historically*, which only *now* suddenly turned sour due to the change to pointer...

It might be a good idea to do a full review of cast (more correctly: BREAK-CAST) activity
in your code:
- why is a BREAK-CAST needed? Can it be avoided or at least minimized,
  via useful structural changes?
- is it an actually BREAKING BREAK-CAST? (resulting in *UNIDENTIFIABLE* bugs such as this one)

I would STRONGLY suggest doing either code changes *or* moving files (f682156a4fc) -
given proper isolation,
the relevant parts likely would have been visible as properly isolated hunks,
thus it's highly likely that
the crucially relevant parts (non-pointer r vs. pointer rnd) would have been
directly pinpointable by reviewers
(and thus likely much earlier than those 40 days - one could say that it
was pure luck that Kamil Dudka managed to catch it this early - congratulations, to both parties!!).

(even git show -M of that commit does not combine the activities,
at least not without similarity index tweaks)

The diffstat in itself isn't that much after all:
 lib/Makefile.inc | 4 +--
 lib/Makefile.vc6 | 1 +
 lib/formdata.c | 9 ++++--
 lib/rand.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/rand.h | 42 ++++++++++++++++++++++++
 lib/vauth/digest.c | 15 +++++----
 lib/vauth/ntlm.c | 12 ++++---
 lib/vtls/gskit.h | 4 +--
 lib/vtls/vtls.c | 84 ++++++-----------------------------------------
 lib/vtls/vtls.h | 9 ++----
 10 files changed, 211 insertions(+), 98 deletions(-)

, thus it is likely after all that
the commit "would have been" (wise-*ss post-mortem words ;-))
still sufficiently nicely reviewable.

This issue might be an incentive to review usual coding behaviour,
to try to further identify/follow Best Practice.

Greetings (and thanks heaps for this infrastructure software!),

Andreas Mohr
-------------------------------------------------------------------
Unsubscribe: https://cool.haxx.se/list/listinfo/curl-library
Etiquette: https://curl.haxx.se/mail/etiquette.html
Received on 2017-02-17