Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 kevin8t8):

 Thanks again Derek.  As always I appreciate your opinion.  I will give
 this some more thought...

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 If I were going to take the time to make this code "nice" I would write a
 function that takes a MUTTMENU *, and some sprintf-like arguments, and
 allocates whatever memory is needed a la snprintf() [Sorry for the lack of
 accents Vincent, hard to type them here.] :)

 Then nearly all of interactive_check_cert() goes away, and is replaced
 with code like:

 {{{
 mutt_menu_add_row(menu, "%s", _("This certificate belongs to:"));
 mutt_menu_add_row(menu, "  %s", x509_get_part(issuer,
 NID_commonName));
 [...]
 }}}
 The cert field bits would still make sense to put in a helper function
 since there is the issuer and the subject, and they use the same fields.
 It would essentially be the same as the helper I already wrote except
 replace calls to snprintf() with calls to mutt_menu_add_row(). No
 counting, no macros, just simple, straightforward, bug-free code.

 For what it's worth, also note that in the current checked-in version of
 the function, none of the calls to snprintf() are checked.  This is
 probably OK since I don't believe any of the fields can be longer than
 that, but it is not robust.  Rows which exceed the size of SHORT_STRING
 would therefore not be added to the menu, resulting in either a corrupted
 or truncated dialog.  That is, the row in question would be skipped and
 its corresponding SHORT_STRING would be filled with NULLs.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 Replying to [comment:27 vinc17]:

 > 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).

 It's a helper function... it doesn't need to.  The two are meant to go
 together.  It's like friend classes in C++.  It's not inherently bad.

 > 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...

 This seems good.  It's more work but if we all agree the code is generally
 not great then that should be a goal anyway, no? :)

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 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. Your comment:19 shows that it's just as easy
 to goof up the helper function.  Both of these goof ups will show up with
 a quick test, but I thought we were more discussing general code clarity
 and maintainability here.

 I was arguing that Vincent's point about underspecifying the array
 essentially applied to both the fixed-length array and the macro cases;
 therefore on that basis specifically, neither solution was better than the
 other--other factors needed to be considered.

 > Again, I completely agree with your comment:10, but if we can avoid
 misuse, there are situations where a mutt_array_size() makes the code
 clearer, more concise, and less buggy.

 I just don't agree.  I think the nested macros are much too hard to parse,
 and apparently depends on a non-standard compiler extension?  Clever code
 is not good code.  Code that is easy to read and understand is good code.

 > 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.

 You could remove that as well.  You could instead have your helper
 function realloc the menu buffer, or you could also use the snprintf()
 trick in the snprintf man page to copy the whole pile of strings at once,
 and do at most one realloc().  The check_cert() function could be
 similarly adjusted...  realloc() a lot of data isn't awesome but we're
 talking about a small number of reallocs with relatively small strings...
 it shouldn't really matter too much.  Or you could use an array of char*
 where each element is a dynamically allocated row, where the last one is
 NULL.  Then the only thing you might need to realloc is additional pointer
 elements.

 There are many ways to solve that problem.

 > I think mutt_array_size() accomplishes this marginally better than the
 helper function.

 I don't agree, but I've said my piece, and it's time to move on.

 For what it's worth, I thought the original patch was adequate.  The only
 reason I'm pursuing this is because Vincent objected for reasons of code
 quality, and I don't think the macro is in any way an improvement.  I
 think writing good code is the right goal; but if you're going to take
 that seriously, then take it seriously. :)

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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:23 derekmartin]:
 > I can't imagine how this is a better option than the helper function
 (which you could inline), which is simpler, clearer, and more efficient,
 avoiding the indirection of the macros, the array, and the looping
 entirely.  With optimizations enabled it probably makes very little
 difference (and certainly wouldn't be noticeable to the user in any event)
 but it's still way, way easier to understand.

 Yes, here, the helper function may be a better idea than the array + loop,
 though I haven't look at that closely.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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:22 kevin8t8]:
 > You can trace it through, but it ends up using a gcc extension
 !__builtin_types_compatible_p.

 You can enable the check only for GCC and compatible compilers such as
 clang. I assume that most developers (if not all) use such a compiler, so
 that any bug due to bad type would be detected very quickly by a
 compilation failure.

 > In any case, if we use Vincent's declaration instead of the gcc
 extension, this gives us a possibility:
 >
 > {{{
 > #define BUILD_BUG_UNLESS_ZERO(e) (sizeof(struct { int:-!!(e); }))
 > #define __must_be_array(a)   BUILD_BUG_UNLESS_ZERO((void *) &(a) !=
 (void *) &(a)[0])
 > #define mutt_array_size(x)   (sizeof (x) / sizeof ((x)[0]) +
 __must_be_array(x))
 > }}}

 I'm not sure that this code is valid ISO C code, because {{{(void *) &(a)
 != (void *) &(a)[0]}}} is not a standard integer constant expression as
 required by the bit-field size (though its value can be determined at
 compile time). So, this would still be an extension of some compilers such
 as GCC.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 kevin8t8):

 I figured out my confusion.  !__must_be_array() in the Linux source was
 comparing "a with [0]", not " with [0]".

 So the meaning of the macro is BUILD_BUG_ON "if the parameter is true",
 "otherwise return" ZERO.

 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.  Your comment:19 shows that it's just as easy
 to goof up the helper function.  Both of these goof ups will show up with
 a quick test, but I thought we were more discussing general code clarity
 and maintainability here.

 Again, I completely agree with your comment:10, but if we can avoid
 misuse, there are situations where a mutt_array_size() makes the code
 clearer, more concise, and less buggy.  I wouldn't want to see it all over
 the code, but for small self-contained functions I think it makes sense.
 Yes, your helper function is perhaps clearer than the loop, but on the
 other hand, the setting of menu->max is more brittle, and easier to forget
 to change if a field is added to the helper.

 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.

 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, and if he has any
 better ideas.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 Personally, I can't even parse that (particularly the
 BUILD_BUG_UNLESS_ZERO macro).  It nests macro upon macro, and if used for
 this problem it does a bunch of math in a loop... all to copy a fixed
 number of fields into a buffer, essentially.  The word that comes to mind
 is "gross."  I can't imagine how this is a better option than the helper
 function (which you could inline), which is simpler, clearer, and more
 efficient, avoiding the indirection of the macros, the array, and the
 looping entirely.  With optimizations enabled it probably makes very
 little difference (and certainly wouldn't be noticeable to the user in any
 event) but it's still way, way easier to understand.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 kevin8t8):

 Damien chimed in on #mutt to point out Linux has something which does
 this: ARRAY_SIZE http://lxr.free-
 electrons.com/source/include/linux/kernel.h#L53.

 You can trace it through, but it ends up using a gcc extension
 !__builtin_types_compatible_p.

 To abort compilation, it uses BUILD_BUG_ON_ZERO.  But I can't figure this
 macro out.  It looks like this macro fails *unless* you pass it zero.  So
 perhaps I'm looking at the wrong instantiation of this or something.
 Hopefully Damien or someone can unconfuse me.

 In any case, if we use Vincent's declaration instead of the gcc extension,
 this gives us a possibility:

 {{{
 #define BUILD_BUG_UNLESS_ZERO(e) (sizeof(struct { int:-!!(e); }))
 #define __must_be_array(a)   BUILD_BUG_UNLESS_ZERO((void *) &(a) != (void
 *) &(a)[0])
 #define mutt_array_size(x)   (sizeof (x) / sizeof ((x)[0]) +
 __must_be_array(x))
 }}}

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Derek Martin]

Taking this out of the bug, since it's no longer really relevant...

On Fri, Dec 02, 2016 at 01:00:08AM -, Mutt wrote:
>  Replying to [comment:18 derekmartin]:
>  > The macro version has the opposite problem: You can make the array too
>  small and it will "work" but it will be incomplete.
> 
>  I don't see any problem.

The problem here is exactly the same problem as with the version I
presented...  In both cases you can under-specify the data.  In both
cases it will still "work" -- you just won't get what you expect.
The cases are different, but there's no *practical* difference.

>  > Ultimately, a bug is a bug, and you just have to write the code
>  correctly.
> 
>  This is more complex than that. 

I don't agree, because:

> Code evolves and can become incorrect. 

This statement is true; however it's the responsibility of whoever
modified it to understand the code and make correct modifications.  If
they failed, they created a bug.  A bug is a bug is a bug.  If someone
modifies code without understanding it, all sorts of chaos might
ensue.

Also, this is an argument AGAINST using macros.  They are less
well-specified than functions, and especially where odd structures and
casts are used to make them generic, are more prone to being used
incorrectly, because the compiler has a lessened ability to ensure
code correctness.  As a result, the macro places a higher burden of
understanding on the maintainer than a properly specified function.

> The advantage of the macro is that the size of the array is given
> only at one place: via the initializer list.

Which makes zero difference to program correctness if the array was
not specified properly.  

The advantage to the fixed length array is the programmer can see
exactly how many elements are SUPPOSED to be in the array, and simply
count them.  But in this case, just as in the macro case, it still
makes no difference to program correctness if the fixed-length array
did not have enough elements assigned (was not specified properly).
In the first case, menu doesn't have enough rows printed (some correct
data is missing).  In the second case, it has the right number of
rows, but row 0 is incorrectly repeated 2 additional times at the end
(some correct data is missing, though replaced with wrong data).  It's
exactly the same class of problem, just slightly different details.

The helper function approach eliminates both of those problems, while
using less code to do it.  It is exactly precise and cannot fail so
long as the SSL library provided valid and correct data to the caller
(which was already required for both of the other two solutions
anyway).

The lesson is:  Prefer functions to macros.  In general, while they
may have their uses, macros suck.

-- 
Derek D. Martinhttp://www.pizzashack.org/   GPG Key ID: 0xDFBEAD02
-=-=-=-=-
This message is posted from an invalid address.  Replying to it will result in
undeliverable mail due to spam prevention.  Sorry for the inconvenience.



pgp36ZROixPdv.pgp
Description: PGP signature

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Derek Martin]

On Fri, Dec 02, 2016 at 06:58:07PM -, Mutt wrote:
> One thing that would tilt me decidedly "in favor" of the macro is if there
> were a way to generate an error or warning at compilation time, (even if
> it was just a gcc extension).  

There's none.  That's exactly the problem with macros.  The logic needs
to be in the macro itself, and that's inherently a run-time solution.

-- 
Derek D. Martinhttp://www.pizzashack.org/   GPG Key ID: 0xDFBEAD02
-=-=-=-=-
This message is posted from an invalid address.  Replying to it will result in
undeliverable mail due to spam prevention.  Sorry for the inconvenience.



pgpoAQLCch2Xm.pgp
Description: PGP signature

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 kevin8t8):

 I think it wasn't fair of me to ask other's opinions in the middle of a
 ticket.  Probably at this point just the three of us are paying attention
 to this thread.

 One thing that would tilt me decidedly "in favor" of the macro is if there
 were a way to generate an error or warning at compilation time, (even if
 it was just a gcc extension).  Vincent, do you know of a way of doing
 this?

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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:18 derekmartin]:
 > The macro version has the opposite problem: You can make the array too
 small and it will "work" but it will be incomplete.

 I don't see any problem.

 > Ultimately, a bug is a bug, and you just have to write the code
 correctly.

 This is more complex than that. Code evolves and can become incorrect. The
 advantage of the macro is that the size of the array is given only at one
 place: via the initializer list.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 Oops, the dialog[] index needs to be (*row)++...

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 Replying to [comment:17 vinc17]:
 > That would be OK, but the code needs to check that the initializer has
 enough elements, as the C standard allows you to give fewer elements than
 the real size (the other ones are initialized to 0).

 The macro version has the opposite problem: You can make the array too
 small and it will "work" but it will be incomplete.  Ultimately, a bug is
 a bug, and you just have to write the code correctly.  The compiler and
 coding conventions can only do so much for you.

 Besides, like I said, the better solution is to forgo both the array and
 the struct replacement, and just write a helper function that does the
 sprintf.  It is more explicit and can't be wrong.

 Add this helper function:

 {{{
 static void sprintf_cert_menu(MUTTMENU *menu, X509_NAME *n, int *row)
 {
 snprintf(menu->dialog[*row++], SHORT_STRING, "  %s", x509_get_part(n,
 NID_commonName);
 snprintf(menu->dialog[*row++], SHORT_STRING, "  %s", x509_get_part(n,
 NID_pkcs9_emailAddress);
 snprintf(menu->dialog[*row++], SHORT_STRING, "  %s", x509_get_part(n,
 NID_organizationName);
 snprintf(menu->dialog[*row++], SHORT_STRING, "  %s", x509_get_part(n,
 NID_organizationalUnitName);
 snprintf(menu->dialog[*row++], SHORT_STRING, "  %s", x509_get_part(n,
 NID_localityName);
 snprintf(menu->dialog[*row++], SHORT_STRING, "  %s", x509_get_part(n,
 NID_stateOrProvince);
 snprintf(menu->dialog[*row++], SHORT_STRING, "  %s", x509_get_part(n,
 NID_countryName);
 }

 }}}

 Then remove the int array part[], and replace both occurances of the loop
 structure to snprintf() the parts with a call to the helper:

 {{{
 sprintf_cert_menu(menu, x509_subject, );
 [...]
 sprintf_cert_menu(menu, x509_issuer, );
 }}}

 Done and done.  Less code, more explicit, no need for any macro
 contortions, can't be wrong (and still compile).  I'm only unsure of
 whether or not the x509 functions can fail, as I'm unable to find man
 pages for x509_get_*_name() functions...

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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:10 derekmartin]:
 > Better would be something like:
 >
 > {{{
 > #define CERT_CHECK_PARTS 7
 > typedef int cert_check_parts[CERT_CHECK_PARTS];
 > [...]
 > const cert_check_parts part = { ... };
 > [...]
 > for (i = 0; i < CERT_CHECK_PARTS; ++i) ...
 > }}}
 That would be OK, but the code needs to check that the initializer has
 enough elements, as the C standard allows you to give fewer elements than
 the real size (the other ones are initialized to 0).

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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:10 derekmartin]:
 > Replying to [comment:2 vinc17]:
 > > {{{
 > > #define numberof(x)  (sizeof (x) / sizeof ((x)[0]))
 > >
 > > for (i = 0; i < numberof(parts); i++)
 > > }}}
 >
 > I think this kind of macro, much like the macro in bug #3880, is bad
 business.  What if it gets used with a pointer rather than an array?  The
 syntax is still fine, it'll compile, but it will break, in a way that may
 be difficult to notice, depending on exactly how it's used.

 The macro can be modified to check whether the variable is a pointer. So,
 I don't see any problem.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 kevin8t8):

 Hi Derek,

 You eloquently voiced my reservations about the construct.  It need not
 only be a less-careful programmer that gets bit by this.  I can imagine
 refactoring a hunk of code out of a large function, turning the direct
 array reference into a function parameter.  It would be easy to miss the
 problem.

 However, I am reluctant to touch this function (interactive_check_cert)
 any more.  My original goal was to fix the X509_NAME_oneline() and
 strstr() issues, which 1a2dc7b21b5b did.

 At this point, I *am* willing to consider reverting 1196c859942e, if
 others also voice a "nay" against the mutt_array_size() macro.  So please
 speak up if you have an opinion.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 FWIW, I think macros should be avoided, in general.  They can cause too
 many problems.  They're OK for small bits of repetitive code in
 performance-critical sections, but even then, you're usually better off
 with an inline function instead.

 Explicit is better than implicit.  Macros make things more implicit, and
 are hard to write in a way where they can always be used in place of an
 equivalent expression or function call, resulting in ridiculous constructs
 to make them sufficiently generalized.  You should almost always just use
 a function or write out the code instead, and limit macro usage to
 eliminating the repeated use of hard-coded literal constants.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 oops, I missed the struct itself:

 {{{
 typedef struct _cert_check_parts {
 char *common_name;
 char *email_addr;
 char *org_name;
 char *org_unit_name;
 char *locality;
 char *state_prov;
 char *country;
 } mutt_cert_check_parts;

 }}}

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 Actually, you could just use a modified version of my printf_cert_menu()
 function that takes the X509_NAME * instead of the mutt_cert_check_parts
 struct...  If you do that, this actually ends up being LESS code, and
 eliminates the int array and the mutt_cert_check_parts struct entirely.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 The struct version would look something like this:

 {{{
 static int get_cert_check_parts(X509_NAME *n, mutt_cert_check_parts *cp)
 {
 buf[STRING];
 /* if this can fail, return FAIL? */
 cp->common_name = x509_get_part(n, NID_commonName);
 cp->email_addr = x509_get_part(n, NID_pkcs9_emailAddress);
 cp->org_name = x509_get_part(n, NID_organizationName);
 cp->org_unit_name = x509_get_part(n, NID_organizationalUnitName);
 cp->locality = x509_get_part(n, NID_localityName);
 cp->state_prov = x509_get_part(n, NID_stateOrProvince);
 cp->country = x509_get_part(n, NID_countryName);
 return SUCCESS;
 }

 /* optionally could take int *row here... */
 static void sprintf_cert_menu(MUTTMENU *menu, mutt_cert_check_parts *cp)
 {
 int row = 0;
 snprintf(menu->dialog[row++], SHORT_STRING, "  %s", cp->common_name);
 snprintf(menu->dialog[row++], SHORT_STRING, "  %s", cp->email_addr);
 snprintf(menu->dialog[row++], SHORT_STRING, "  %s", cp->org_name);
 snprintf(menu->dialog[row++], SHORT_STRING, "  %s",
 cp->org_unit_name);
 snprintf(menu->dialog[row++], SHORT_STRING, "  %s", cp->locality);
 snprintf(menu->dialog[row++], SHORT_STRING, "  %s", cp->state_prov);
 snprintf(menu->dialog[row++], SHORT_STRING, "  %s", cp->country);
 }

 static int interactive_cert_check(...)
 {
 X509_NAME *issuer;
 X509_NAME *subject;
 mutt_cert_check_parts issuer_parts;
 mutt_cert_check_parts subject_parts;

 [...]

 issuer = X509_get_issuer_name(cert);
 /* handle error if appropriate */
 get_cert_check_parts(issuer, _parts);
 sprintf_cert_menu(menu, _parts);

 subject = X509_get_subject_name(cert);
 /* handle error if appropriate */
 get_cert_check_parts(subject, _parts);
 sprintf_cert_menu(menu, _parts);
 [...]

 }

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 derekmartin):

 Replying to [comment:2 vinc17]:
 > {{{
 > #define numberof(x)  (sizeof (x) / sizeof ((x)[0]))
 >
 > for (i = 0; i < numberof(parts); i++)
 > }}}

 I think this kind of macro, much like the macro in bug #3880, is bad
 business.  What if it gets used with a pointer rather than an array?  The
 syntax is still fine, it'll compile, but it will break, in a way that may
 be difficult to notice, depending on exactly how it's used.

 {{{
 $ cat foo.c
 #include 

 #define  n(x) (sizeof (x) / sizeof ((x)[0]))

 int main(int argc, char **argv)
 {
 long int a[4];
 long int *b = a;

 printf("sizeof a = %ld, size of a[0] = %ld\n", sizeof a, sizeof a[0]);
 printf("n(a) = %ld\n", n(a));
 printf("sizeof b = %ld, size of b[0] = %ld\n", sizeof b, sizeof b[0]);
 printf("n(b) = %ld\n", n(b));
 return 0;
 }

 $ ./foo
 sizeof a = 32, size of a[0] = 8
 n(a) = 4
 sizeof b = 8, size of b[0] = 8
 n(b) = 1

 }}}

 Not cool.  It's been my experience that many less-careful programmers will
 see a construct used and try to reuse it without really understanding
 it...  If they use this with data that just happens to be of the right
 size it'll seem to work, but in most cases it will break.

 Better would be something like:

 {{{
 #define CERT_CHECK_PARTS 7
 typedef int cert_check_parts[CERT_CHECK_PARTS];
 [...]
 const cert_check_parts = { ... };
 [...]
 for (i = 0; i < CERT_CHECK_PARTS; ++i) ...
 }}}


 Better still would be to use a struct instead of the int array, and define
 helper functions to define the struct and to sprintf the struct into a
 buffer, since those actions are repeated.  It's only slightly more code,
 but WAY more explicit and much safer.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--

Comment (by kevin8t8):

 Thanks Vincent.  I think the comment is enough.  Perhaps I'm being too
 cautious, and the warning isn't even necessary.  I'll push these two
 patches up shortly.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--

Comment (by vinc17):

 {{{
 +/* Use this with care.  If the compiler can't see the array
 + * definition, it obviously won't produce a correct result */
 +#define mutt_array_size(x)  (sizeof (x) / sizeof ((x)[0]))
 }}}
 If you fear of incorrect use, one can add a check that {{{sizeof (x)}}} is
 a multiple of {{{sizeof ((x)[0])}}}, though this wouldn't be very useful
 in practice. Also comparing the variable size {{{sizeof (x)}}} and the
 element size {{{sizeof (&(x)[0])}}} may be useful, but it would yield an
 error for 1-element arrays whose size is the same as the pointer size;
 however, when this macro is used, the array probably has more than one
 element, so that this should be safe.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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:
---+--
Changes (by Kevin McCarthy ):

 * status:  new => closed
 * resolution:   => fixed


Comment:

 In [changeset:"1a2dc7b21b5b93806f6e63447e5014f009e98a54"
 6876:1a2dc7b21b5b]:
 {{{
 #!CommitTicketReference repository=""
 revision="1a2dc7b21b5b93806f6e63447e5014f009e98a54"
 Improve openssl interactive_check_cert. (closes #3899)

 Don't use X509_NAME_oneline() with a fixed size buffer, which could
 truncate the string, perhaps leaving off the CN field entirely.
 Instead, work directly off the X509_NAME.

 Rather than use strstr to tokenize it, call
 X509_NAME_get_text_by_NID() with the nid types.  Although
 X509_NAME_get_text_by_NID() is "legacy", it is the most directly
 useful for mutt in this simple interactive prompt.

 The function was set up to include the ST and C fields in the prompt,
 but the loop limit was too low.  I believe this was an oversight, so
 increase the loop to include those two fields.
 }}}

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#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 Kevin McCarthy ):

 In [changeset:"1196c859942e393d353c8df77d680c7a44e76389"
 6877:1196c859942e]:
 {{{
 #!CommitTicketReference repository=""
 revision="1196c859942e393d353c8df77d680c7a44e76389"
 Add mutt_array_size macro, change interactive_check_cert() to use it.
 (see #3899)

 While I have reservations about the construct, it does make the
 interactive_check_cert() menu->max and part loop less fragile.
 }}}

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--

Comment (by vinc17):

 Replying to [comment:5 vinc17]:
 > If you fear of incorrect use, one can add a check that {{{sizeof (x)}}}
 is a multiple of {{{sizeof ((x)[0])}}}, [...]

 Or better, check that {{{(void *) &(x) == (void *) &(x)[0]}}}.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--
Changes (by kevin8t8):

 * Attachment "ticket-3899-part2.patch" added.


--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--

Comment (by kevin8t8):

 Adding a part two patch that adds a macro mutt_array_size to lib.h, and
 changes interactive_cert_check() so use it.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--

Comment (by kevin8t8):

 Funnily enough I just asked Damien not to use that construct, because
 we're not generally using it in mutt.  Now that I look closer, I do see a
 couple places in init.c and keymap.c, but there is still something I don't
 like about it...

 A problem is the menu structure is fragile too.  It currently hard codes
 the max size and pre-allocates it.  So if you increase the number of
 parts, you have to remember to increase the max size right now.  I didn't
 want to change too much in the function, so I took the easy way out.

 Perhaps a compromise is to set
   menu->max = numberof(parts) * 2 + 9;
 for now.

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--

Comment (by vinc17):

 I think that the number of fields in the two loops
 {{{for (i = 0; i < 7; i++)}}}
 should not be hardcoded, but automatically obtained from the real number
 of fields, with something like:
 {{{
 #define numberof(x)  (sizeof (x) / sizeof ((x)[0]))

 for (i = 0; i < numberof(parts); i++)
 }}}

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--
Changes (by kevin8t8):

 * Attachment "ticket-3899.patch" added.


--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3899: mutt_ssl's interactive_check_cert() has several issues

[Mutt]

#3899: mutt_ssl's interactive_check_cert() has several issues
---+--
  Reporter:  kevin8t8  |  Owner:  mutt-dev
  Type:  defect| Status:  new
  Priority:  major |  Milestone:
 Component:  crypto|Version:
Resolution:|   Keywords:
---+--

Comment (by kevin8t8):

 Attaching a proposed patch.

--
Ticket URL: 
Mutt 
The Mutt mail user agent