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

mutt: Move '@' pattern modifier documentation to the right section.

[Brendan Cully]

changeset: 6888:df1d1e379477
user:  Kevin McCarthy 
date:  Tue Dec 06 19:07:32 2016 -0800
link:  http://dev.mutt.org/hg/mutt/rev/df1d1e379477

Move '@' pattern modifier documentation to the right section.

Somehow, the patch got out of date and the documentation shifted to
another section.  Relocate back to the "Pattern Modifier" section.

diffs (63 lines):

diff -r d930e39ec095 -r df1d1e379477 doc/manual.xml.head
--- a/doc/manual.xml.head   Sun Dec 04 16:04:29 2016 -0800
+++ b/doc/manual.xml.head   Tue Dec 06 19:07:32 2016 -0800
@@ -4359,29 +4359,6 @@
 macro's commands into its history.
 
 
-
-You can restrict address pattern matching to aliases that you have
-defined with the "@" modifier.  This example matches messages whose
-recipients are all from Germany, and who are known to your alias list.
-
-
-
-
-^@~C \.de$
-
-
-
-
-To match any defined alias, use a regular expression that matches any
-string.  This example matches messages whose senders are known aliases.
-
-
-
-
-@~f .
-
-
-
 
 
 
@@ -5221,6 +5198,29 @@
 
 
 
+
+You can restrict address pattern matching to aliases that you have
+defined with the "@" modifier.  This example matches messages whose
+recipients are all from Germany, and who are known to your alias list.
+
+
+
+
+^@~C \.de$
+
+
+
+
+To match any defined alias, use a regular expression that matches any
+string.  This example matches messages whose senders are known aliases.
+
+
+
+
+@~f .
+
+
+
 
 
 

Re: [Mutt] #3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8

[Mutt]

#3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8
--+-
  Reporter:  william  |  Owner:  brendan
  Type:  defect   | Status:  new
  Priority:  major|  Milestone:
 Component:  IMAP |Version:
Resolution:   |   Keywords:
--+-

Comment (by william):

 It still says 'no' for that first line, but then barfs on the next one as
 well:
 {{{
 checking for X509_STORE_CTX_new in -lcrypto... no
 checking for SSL_new in -lssl... no
 configure: error: Unable to find SSL library

 [...]
 configure:10079: result: no
 configure:10085: checking for SSL_new in -lssl
 configure:10110: gcc -o conftest -g -O2   conftest.c -lssl -lz  >&5
 ld: library not found for -lssl
 clang: error: linker command failed with exit code 1 (use -v to see
 invocation)
 configure:10110: $? = 1
 }}}

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8

[Mutt]

#3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8
--+-
  Reporter:  william  |  Owner:  brendan
  Type:  defect   | Status:  new
  Priority:  major|  Milestone:
 Component:  IMAP |Version:
Resolution:   |   Keywords:
--+-

Comment (by kevin8t8):

 Looks like changeset 2c1d79d3edd5 added in an an AC_MSG_ERROR() to the
 [action-if-not-found] action when checking for -lcrypto.  I actually
 didn't notice that, and I wonder if that's the problem.

 Can you try this patch?

--
Ticket URL: 
Mutt 
The Mutt mail user agent

[Mutt] #3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8

[Mutt]

#3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8
-+-
 Reporter:  william  |  Owner:  brendan
 Type:  defect   | Status:  new
 Priority:  major|  Milestone:
Component:  IMAP |Version:
 Keywords:   |
-+-
 1.7.0 built fine for me.

 With 1.7.2, I'm getting this error, despite having an OpenSSL version
 above the one listed in the announcement.

 {{{
 jazz~/mutt-1.7.2 % openssl version
 OpenSSL 0.9.8zh 14 Jan 2016
 }}}

 in config.log:
 {{{
 configure:10091: result: no
 configure:10096: error: Unable to find SSL library
 }}}

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [Mutt] #3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8

[Mutt]

#3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8
--+-
  Reporter:  william  |  Owner:  brendan
  Type:  defect   | Status:  new
  Priority:  major|  Milestone:
 Component:  IMAP |Version:
Resolution:   |   Keywords:
--+-

Comment (by william):

 Kevin -- running that against 1.7.2 release seems to fail?
 {{{
 jazz~/mutt-1.7.2 % patch < ../ticket-3309.patch
 patching file init.h
 Hunk #1 FAILED at 2375.
 1 out of 1 hunk FAILED -- saving rejects to file init.h.rej
 patching file mutt.h
 Hunk #1 FAILED at 404.
 1 out of 1 hunk FAILED -- saving rejects to file mutt.h.rej
 patching file rfc3676.c
 Hunk #1 FAILED at 56.
 Hunk #2 FAILED at 150.
 Hunk #3 FAILED at 221.
 3 out of 3 hunks FAILED -- saving rejects to file rfc3676.c.rej
 }}}
 is the patch against current head maybe?

--
Ticket URL: 
Mutt 
The Mutt mail user agent

Re: [PATCH 3/4] make Muttrc a local variable

[Derek Martin]

On Sat, Dec 03, 2016 at 08:54:00PM -0500, Damien Riegel wrote:
> This variable is only used to pass the '-F' argument from main to
> mutt_init, so it can be changed to be local.

From a code-management perspective, I don't think this is a good
change, mainly for the sake of consistency.  Mutt's various parameters
and configurations are global so making the muttrc not global breaks
expectations, and doesn't really serve any other useful purpose.  It's
a ~150-line diff for very little practical benefit.

While we may not have such a thing now, one can imagine that it might
be nice to have a "reread_muttrc" command--for example if the user has
been experimenting with custom settings, such a command could be used
as a shortcut to dump the existing configuration and reload from the
previously specified Muttrc.

I don't really like the idea of passing multiple muttrcs on the
command line either--the user can already include multiple files
inside their .muttrc or read additional files from the command prompt.
It's not a horrible idea, but it seems unnecessary and breaks with the
above.  I can't really think of any other user-configurable programs
that let you pass multiple configs on the command line either.  It
strikes me as more confusing than useful.

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



pgp_b6omrnTNo.pgp
Description: PGP signature

Re: [Mutt] #3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8

[Mutt]

#3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8
--+-
  Reporter:  william  |  Owner:  brendan
  Type:  defect   | Status:  new
  Priority:  major|  Milestone:
 Component:  IMAP |Version:
Resolution:   |   Keywords:
--+-
Changes (by kevin8t8):

 * Attachment "ticket-3902.patch" added.


--
Ticket URL: 
Mutt 
The Mutt mail user agent

OSX OpenSSL

[Will Yardley]

On Sun, Dec 04, 2016 at 06:25:38PM -0800, Kevin J. McCarthy wrote:
> 
> Please note that as part of the OpenSSL fixes, versions less than
> 0.9.6 are no longer supported.

I get a problem with OpenSSL 0.9.8zh on OSX 10.11.6.

I created https://dev.mutt.org/trac/ticket/3902

Happy to try and debug this if anyone can help.

w

Re: [Mutt] #3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8

[Mutt]

#3902: build problem on Mac OS X 10.11 with OpenSSL 0.9.8
--+-
  Reporter:  william  |  Owner:  brendan
  Type:  defect   | Status:  new
  Priority:  major|  Milestone:
 Component:  IMAP |Version:
Resolution:   |   Keywords:
--+-
Changes (by william):

 * Attachment "config.log.txt" added.

 config.log

--
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: [PATCH 1/4] Additionally search Muttrc in XDG_CONFIG_DIRS

[Kevin J. McCarthy]

On Sat, Dec 03, 2016 at 08:53:58PM -0500, Damien Riegel wrote:
> To comply with XDG Base Directory Specification, look for Muttrc in
> folders listed in XDG_CONFIG_DIRS.
> ---
>  init.c| 58 ++
>  muttbug.sh.in |  3 ++-
>  2 files changed, 48 insertions(+), 13 deletions(-)

Concatenating all the combined dirs into buffer (size STRING) is
probably not a great idea.

In fact, although I didn't catch it, we should really be using a buffer
of size _POSIX_PATH_MAX in mutt_find_cfg().

I'd like to see the files[] inside of mutt_find_sys_cfg(), instead of
exposed like that.

Quickly commenting on the other patches here.  In patch 2, you shouldn't
move the overwrite of AliasFile after you have sourced the system
muttrc.  I'm not crazy about either patches 3 and 4, but will think
about them.

-- 
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA


signature.asc
Description: PGP signature