#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 vinc17):
Replying to [comment:24 kevin8t8]:
> Derek, I understand your general point, but I don't follow your logic in
comment:18. Saying you can just as easily make the array too small just
sounds argumentative to me.
With this helper function, doesn't the array disappear completely? However
the value of {{{menu->max}}} would depend on the contents of the helper
function, which is quite bad (the helper function would no longer be a
black box from the outside). Now, even with the current code, it is not
obvious that {{{menu->max}}} is large enough. I mean that one can easily
miss a {{{row++}}}. That's there that bound checking would be useful to
avoid a buffer overflow in case of bug.
> Your comment:19 shows that it's just as easy to goof up the helper
function.
Here, this is just a C error like it can happen everywhere. However, there
should have been a warning from GCC due to missing parentheses so that the
precedence becomes explicit.
> Now, that said, I think hardcoding the menu->max beforehand is crap
code, but I'm trying to minimize my changes to this function, make it a
little better maintainable for the future, and move on. I think
mutt_array_size() accomplishes this marginally better than the helper
function.
Yes, I think that due to the {{{menu->max}}} issue,
{{{mutt_array_size()}}} is currently better. But perhaps things can be
improved again...
> Changing topics to the macro, the "BUILD_BUG" macro is pretty clever.
It takes the size of a struct with a bit field. A zero-length bit field
will end up returning a 0 size, but a negative bit field length is illegal
and will generate a compiler error.
>
> The "must_be_array" uses Vincents comparison to make sure "a" is an
array and a not an array pointer.
>
> The mutt_array_size() is modified to add the result of "must_be_array",
which will either return 0 or will generate a compilation error.
>
> It's a little complicated, but not too bad (with a few comments thrown
in). I'm curious what Vincent's feedback is though,
This is based on a GCC extension, though. This means that the usual
{{{__GNUC__}}} test should be done to enable the check.
> and if he has any better ideas.
To avoid {{{menu->max}}}, a different structure could be used than a
fixed-size array for {{{menu->dialog}}}, and IMHO, this should be hidden
in {{{menu.c}}}, everything being handled via function calls. Well, this
would mean to change a large part of the code...
--
Ticket URL: <https://dev.mutt.org/trac/ticket/3899#comment:27>
Mutt <http://www.mutt.org/>
The Mutt mail user agent
