#3899: mutt_ssl's interactive_check_cert() has several issues
-----------------------+----------------------
Reporter: kevin8t8 | Owner: mutt-dev
Type: defect | Status: closed
Priority: major | Milestone:
Component: crypto | Version:
Resolution: fixed | Keywords:
-----------------------+----------------------
Comment (by dgc):
I don't mean to swim more than a lap or two, but I did want to dive in
with some observations - both on code and on process:
First, let's get the issues fixed and move on.
If we have ideas for improving, by all means someone willing to put in the
time should propose a patch. But let's not bikeshed the first round at the
cost of the person who's willing to write committable code. Derek's and
Vincent's contributions are quite valuable, and both are deeply respected
team members, and this is a good discussion. But the way it has evolved,
any alternatives to Kevin's patch depend on Kevin to write, and he's
already addressed the problem once. So let's let Kevin's current work
stand if it's adequate and propose "nice" for the next go-round.
I do like Vincent's suggestion at the top of this ticket for dynamically
accomodating the data we have. The (sizeof()/sizeof()) macro is a well
founded approach. It's used in a lot of code. I don't think there's
anything to shy away from in it. It's not an ideal solution, but it's
reliable and simple, and as raw as it feels, it's quite common for C.
Concerns about future misuse can be addressed with a few well placed
comments. Add an #undef afterwards and you have a helper macro with all
the scoping merits of a helper function, and nobody will just come pick it
up and use it randomly in unrelated code.
I agree with avoiding compiler-specific behavior if possible. It might
solve a problem now, but it creates more porting or exception-managing
tasks later. The Linux kernel is pinned to gcc for specific reasons; we're
not, and we have no need to make that choice.
Hairy macros are not bad in themselves — any C library or kernel is filled
with them, and if they offer value I wouldn't dismiss them just because
they're hard to read. That said, I think the original approach plus the
sizeof macro is more than sufficient to obviate these, and keep the code
straightforward for newcomers and maintainers. I would go with that and I
guess I'm -0 on the type-checking macro patch that came after. I'm just
not sure its benefits outweigh its complexity.
Getting some nice reusable code for managing cert overviews, as Derek
illustrated above, seems -- well, nice. But it's akin to developing a
toolkit for dynamically resizable string buffers and raft of string.h
analogues for manipulating them. It's great when you're done, but
sometimes you're in a well bounded situation and you just want to
vsnprintf something without writing a library. If someone wants to come
later and add that library, we can cheer them on, but right now let's
focus on problems we already have.
Finally, I guess I'm not worried about the risk of some hapless future
mutt coder absentmindedly using mutt_array_size() for a non-array. This is
C. Our best protections are documentation and review -- that is,
communication -- and basic caution. There's a trade-off we make to develop
at a reasonable pace in a highly performant language. If we want stronger
type safety as a goal, there's a lot more work to do than this and
focusing overmuch on one small issue is a misplaced effort.
If we really want to commit some effort to improving mutt's runtime safety
and audit, then I think the better first step is to look at APR or
whatever other low-level utility libraries, and start adapting mutt to use
those families in place of mutt's particular implementations. We'll get a
lot more for less spend than by trying to catch tiny fish one by one with
our hands.
--
Ticket URL: <https://dev.mutt.org/trac/ticket/3899#comment:32>
Mutt <http://www.mutt.org/>
The Mutt mail user agent
