[git] GpgOL - branch, master, updated. gpgol-1.4.0-185-gf9efc49
by Andre Heinecke
cvs at cvs.gnupg.org
Mon Nov 28 17:05:18 CET 2016
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GnuPG extension for MS Outlook".
The branch, master has been updated
via f9efc49ffaf5302b680c17ef0eede636b62319e1 (commit)
via 27c65853abdcccf7d739a7e2b09d7910641ac011 (commit)
via c9775be375292e1f268937c099ed8bf9b994b771 (commit)
from f098d08f7bb700e8b1e4868838c79aab844e3ffa (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit f9efc49ffaf5302b680c17ef0eede636b62319e1
Author: Andre Heinecke <aheinecke at intevation.de>
Date: Mon Nov 28 16:45:26 2016 +0100
Rework close and remove hacks for bad events
* src/mail.cpp (Mail::close): Make it static to handle the
case were close causes a mail deletion.
(Mail::close_inspector): Make it static.
(Mail::close_all_mails): Close inspector first. Handle
map changes and object deletion.
(Mail::~Mail): Don't delete event sink.
(Mail::get_close_triggered, Mail::set_close_triggered): New.
* src/mailitem-events.h (EVENT_SINK_INVOKE): Remove
uneccessary Close hacks. Fix fallthrough from Open to
before read. Fix fallthrough from Close to unload.
Check if a close was triggered by us.
(request_close, request_decrypt): Remvoed.
* src/windowmessages.cpp, src/windowmessages.h (REQUEST_DECRYPT)
(REQUEST_CLOSE): Removed.
--
This fixes various inconsistentcies in the Close handling, especially
if the Object calling the close was deleted during the call. Also
closing the inspectors before closing the mail fixes a crash that
could be triggered if more then one mail was open when closing.
The proper closing now also prevents the necessity for decrypt again.
The Open to BeforeRead fallthrough should not be a big issue,
still a problem. And the close to unload fallthrough with my
long comments explaining it,.. well *facepalm*..
GnuPG-Bug-Id: 2855
diff --git a/src/mail.cpp b/src/mail.cpp
index 25d851a..b294f16 100644
--- a/src/mail.cpp
+++ b/src/mail.cpp
@@ -75,6 +75,7 @@ Mail::Mail (LPDISPATCH mailitem) :
m_is_smime_checked(false),
m_is_signed(false),
m_is_valid(false),
+ m_close_triggered(false),
m_moss_position(0),
m_sender(NULL),
m_type(MSGTYPE_UNKNOWN)
@@ -859,17 +860,18 @@ Mail::close_all_mails ()
int err = 0;
std::map<LPDISPATCH, Mail *>::iterator it;
TRACEPOINT;
- for (it = g_mail_map.begin(); it != g_mail_map.end(); ++it)
+ std::map<LPDISPATCH, Mail *> mail_map_copy = g_mail_map;
+ for (it = mail_map_copy.begin(); it != mail_map_copy.end(); ++it)
{
if (!it->second->is_crypto_mail())
{
continue;
}
- if (it->second->close ())
+ if (close_inspector (it->second) || close (it->second))
{
log_error ("Failed to close mail: %p ", it->first);
/* Should not happen */
- if (it->second->revert())
+ if (is_valid_ptr (it->second) && it->second->revert())
{
err++;
}
@@ -1029,9 +1031,9 @@ Mail::get_recipients() const
}
int
-Mail::close_inspector ()
+Mail::close_inspector (Mail *mail)
{
- LPDISPATCH inspector = get_oom_object (m_mailitem, "GetInspector");
+ LPDISPATCH inspector = get_oom_object (mail->item(), "GetInspector");
HRESULT hr;
DISPID dispid;
if (!inspector)
@@ -1066,8 +1068,9 @@ Mail::close_inspector ()
return 0;
}
+/* static */
int
-Mail::close ()
+Mail::close (Mail *mail)
{
VARIANT aVariant[1];
DISPPARAMS dispparams;
@@ -1079,24 +1082,27 @@ Mail::close ()
dispparams.cNamedArgs = 0;
log_oom_extra ("%s:%s: Invoking close for: %p",
- SRCNAME, __func__, this);
- int rc = invoke_oom_method_with_parms (m_mailitem, "Close",
- NULL, &dispparams);
-
- /* Reset the uuid after discarding all changes in the oom
- so that we can still find ourself. */
- set_uuid ();
+ SRCNAME, __func__, mail->item());
+ mail->set_close_triggered (true);
+ int rc = invoke_oom_method_with_parms (mail->item(), "Close",
+ NULL, &dispparams);
- if (!rc)
- {
- /* Now that we have closed it with discard changes we no
- longer need to wipe the mail because the plaintext was
- discarded. */
- m_needs_wipe = false;
- }
+ log_debug ("returned from invoke");
return rc;
}
+void
+Mail::set_close_triggered (bool value)
+{
+ m_close_triggered = value;
+}
+
+bool
+Mail::get_close_triggered () const
+{
+ return m_close_triggered;
+}
+
static const UserID
get_uid_for_sender (const Key k, const char *sender)
{
diff --git a/src/mail.h b/src/mail.h
index b0d5c59..3ca144a 100644
--- a/src/mail.h
+++ b/src/mail.h
@@ -210,11 +210,11 @@ public:
*/
bool is_smime ();
- /** @brief closes the inspector for this mail
+ /** @brief closes the inspector for a mail
*
* @returns true on success.
*/
- int close_inspector ();
+ static int close_inspector (Mail *mail);
/** @brief get the associated parser.
only valid while the actual parsing happens. */
@@ -276,11 +276,17 @@ public:
/** Call close with discard changes to discard
plaintext. returns the value of the oom close
- call. */
- int close ();
+ call. This may have delete the mail if the close
+ triggers an unload.
+ */
+ static int close (Mail *mail);
/** Try to locate the keys for all recipients */
void locate_keys();
+
+ /** State variable to check if a close was triggerd by us. */
+ void set_close_triggered (bool value);
+ bool get_close_triggered () const;
private:
void update_categories ();
void update_body ();
@@ -295,7 +301,8 @@ private:
m_is_smime, /* This is an smime mail. */
m_is_smime_checked, /* it was checked if this is an smime mail */
m_is_signed, /* Mail is signed */
- m_is_valid; /* Mail is valid signed. */
+ m_is_valid, /* Mail is valid signed. */
+ m_close_triggered; /* We have programtically triggered a close */
int m_moss_position; /* The number of the original message attachment. */
char *m_sender;
msgtype_t m_type; /* Our messagetype as set in mapi */
diff --git a/src/mailitem-events.cpp b/src/mailitem-events.cpp
index 4091f06..33b48e9 100644
--- a/src/mailitem-events.cpp
+++ b/src/mailitem-events.cpp
@@ -78,9 +78,6 @@ BEGIN_EVENT_SINK(MailItemEvents, IDispatch)
private:
Mail * m_mail; /* The mail object related to this mailitem */
bool m_send_seen; /* The message is about to be submitted */
- bool m_decrypt_after_write;
- bool m_ignore_unloads;
- bool m_ignore_next_unload;
};
MailItemEvents::MailItemEvents() :
@@ -89,10 +86,7 @@ MailItemEvents::MailItemEvents() :
m_cookie(0),
m_ref(1),
m_mail(NULL),
- m_send_seen (false),
- m_decrypt_after_write(false),
- m_ignore_unloads(false),
- m_ignore_next_unload(false)
+ m_send_seen (false)
{
}
@@ -104,32 +98,6 @@ MailItemEvents::~MailItemEvents()
gpgol_release (m_object);
}
-static DWORD WINAPI
-request_decrypt (LPVOID arg)
-{
- log_debug ("%s:%s: requesting decrypt again for: %p",
- SRCNAME, __func__, arg);
- if (do_in_ui_thread (REQUEST_DECRYPT, arg))
- {
- log_debug ("%s:%s: second decrypt failed for: %p",
- SRCNAME, __func__, arg);
- }
- return 0;
-}
-
-static DWORD WINAPI
-request_close (LPVOID arg)
-{
- log_debug ("%s:%s: requesting close for: %s",
- SRCNAME, __func__, (char*) arg);
- if (do_in_ui_thread (REQUEST_CLOSE, arg))
- {
- log_debug ("%s:%s: close request failed for: %s",
- SRCNAME, __func__, (char*) arg);
- }
- return 0;
-}
-
static bool propchangeWarnShown = false;
/* The main Invoke function. The return value of this
@@ -182,11 +150,7 @@ EVENT_SINK_INVOKE(MailItemEvents)
}
set_gpgol_draft_info_flags (message, draft_flags);
gpgol_release (message);
-
- if (m_mail->is_crypto_mail())
- {
- m_ignore_unloads = true;
- }
+ break;
}
case BeforeRead:
{
@@ -281,10 +245,6 @@ EVENT_SINK_INVOKE(MailItemEvents)
problem of a revert that the mail is created by outlook and
e.g. multipart/signed signatures from most MUA's are broken.
- Close -> discard changes -> then setting the property and
- then saving also works but then the mail is closed / unloaded
- and we can't decrypt again.
-
Some things to try out might be the close approach and then
another open or a selection change. But for now we just warn.
@@ -407,14 +367,6 @@ EVENT_SINK_INVOKE(MailItemEvents)
m_mail->encrypt_sign ();
return S_OK;
}
- else if (m_decrypt_after_write)
- {
- char *uuid = strdup (m_mail->get_uuid ().c_str());
- HANDLE thread = CreateThread (NULL, 0, request_decrypt,
- (LPVOID) uuid, 0, NULL);
- CloseHandle (thread);
- m_decrypt_after_write = false;
- }
break;
}
case Close:
@@ -428,20 +380,8 @@ EVENT_SINK_INVOKE(MailItemEvents)
(Which would save the decrypted data without an event to
prevent it) we cancel the close and then either close it
with discard changes or revert / save it.
- This happens with a window message as we can't invoke close from
+ Contrary to documentation we can invoke close from
close.
-
- But as a side effect the mail, if opened in the explorer still will
- be reverted, too. So shown as empty. To prevent that
- we request a decrypt in the AfterWrite event which checks if the
- message is opened in the explorer. If not it destroys the mail.
-
- Evil Hack: Outlook sends an Unload event after the message is closed
- This is not true our Internal Object is kept alive if it is opened
- in the explorer. So we ignore the unload event and then check in
- the window message handler that checks for decrypt again if the
- mail is currently open in the active explorer. If not we delete our
- Mail object so that the message is released.
*/
if (parms->cArgs != 1 || parms->rgvarg[0].vt != (VT_BOOL | VT_BYREF))
{
@@ -450,64 +390,30 @@ EVENT_SINK_INVOKE(MailItemEvents)
SRCNAME, __func__);
break;
}
+ if (m_mail->get_close_triggered ())
+ {
+ /* Our close with discard changes, pass through */
+ m_mail->set_close_triggered (false);
+ return S_OK;
+ }
*(parms->rgvarg[0].pboolVal) = VARIANT_TRUE;
log_oom_extra ("%s:%s: Canceling close event.",
SRCNAME, __func__);
- m_decrypt_after_write = true;
- m_ignore_unloads = false;
- m_ignore_next_unload = true;
-
- char *uuid = strdup (m_mail->get_uuid ().c_str());
- HANDLE thread = CreateThread (NULL, 0, request_close,
- (LPVOID) uuid, 0, NULL);
- CloseHandle (thread);
+ if (Mail::close(m_mail))
+ {
+ log_debug ("%s:%s: Close request failed.",
+ SRCNAME, __func__);
+ }
}
+ return S_OK;
}
case Unload:
{
log_oom_extra ("%s:%s: Unload : %p",
SRCNAME, __func__, m_mail);
- /* Unload. Experiments have shown that this does not
- mean a mail is actually unloaded in Outlook. E.g.
- If it was open in an inspector and then closed we
- see an unload event but the mail is still shown in
- the explorer. Fun. On the other hand if a message
- was opened and the explorer selection changes
- we also get an unload but the mail is still open.
-
- Really we still get events after the unload and
- can make changes to the object.
-
- In case the mail was opened m_ignore_unloads is set
- to true so the mail is not removed when the message
- selection changes. As close invokes decrypt_again
- the mail object is removed there when the explorer
- selection changed.
-
- In case the mail was closed m_ignore_next_unload
- is set so only the Unload thad follows the canceled
- close is ignored and not the unload that comes from
- our then triggered close (save / discard).
-
-
- This is horribly hackish and feels wrong. But it
- works.
- */
- if (m_ignore_unloads || m_ignore_next_unload)
- {
- if (m_ignore_next_unload)
- {
- m_ignore_next_unload = false;
- }
- log_debug ("%s:%s: Ignoring unload for message: %p.",
- SRCNAME, __func__, m_object);
- }
- else
- {
- log_debug ("%s:%s: Removing Mail for message: %p.",
- SRCNAME, __func__, m_object);
- delete m_mail;
- }
+ log_debug ("%s:%s: Removing Mail for message: %p.",
+ SRCNAME, __func__, m_object);
+ delete m_mail;
return S_OK;
}
default:
diff --git a/src/windowmessages.cpp b/src/windowmessages.cpp
index 64e4ef1..a4bf62c 100644
--- a/src/windowmessages.cpp
+++ b/src/windowmessages.cpp
@@ -54,65 +54,6 @@ gpgol_window_proc (HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
mail->parsing_done();
break;
}
- case (REQUEST_DECRYPT):
- {
- char *uuid = (char *) ctx->data;
- auto mail = Mail::get_mail_for_uuid (uuid);
- if (!mail)
- {
- log_debug ("%s:%s: Decrypt again for uuid which is gone.",
- SRCNAME, __func__);
- xfree (uuid);
- break;
- }
- /* Check if we are still in the active explorer. */
- LPDISPATCH mailitem = get_oom_object (GpgolAddin::get_instance()->get_application (),
- "ActiveExplorer.Selection.Item(1)");
- if (!mailitem)
- {
- log_debug ("%s:%s: Decrypt again but no selected mailitem.",
- SRCNAME, __func__);
- xfree (uuid);
- delete mail;
- break;
- }
-
- char *active_uuid = get_unique_id (mailitem, 0, nullptr);
- if (!active_uuid || strcmp (active_uuid, uuid))
- {
- log_debug ("%s:%s: UUID mismatch",
- SRCNAME, __func__);
- xfree (uuid);
- delete mail;
- break;
- }
- log_debug ("%s:%s: Decrypting %s again",
- SRCNAME, __func__, uuid);
- xfree (uuid);
- xfree (active_uuid);
-
- mail->decrypt_verify ();
- break;
- }
- case (REQUEST_CLOSE):
- {
- char *uuid = (char *) ctx->data;
- auto mail = Mail::get_mail_for_uuid (uuid);
- if (!mail)
- {
- log_debug ("%s:%s: Close request for uuid which is gone.",
- SRCNAME, __func__);
- break;
- }
- if (mail->close())
- {
- log_debug ("%s:%s: Close request failed.",
- SRCNAME, __func__);
- }
- ctx->wmsg_type = REQUEST_DECRYPT;
- gpgol_window_proc (hWnd, message, wParam, (LPARAM) ctx);
- break;
- }
case (INVALIDATE_UI):
{
log_debug ("%s:%s: Invalidating UI",
diff --git a/src/windowmessages.h b/src/windowmessages.h
index 7c07ae3..4dbd711 100644
--- a/src/windowmessages.h
+++ b/src/windowmessages.h
@@ -39,9 +39,6 @@ typedef enum _gpgol_wmsg_type
INVALIDATE_UI = 1, /* The UI should be invalidated. */
PARSING_DONE = 2, /* A mail was parsed. Data should be a pointer
to the mail object. */
- REQUEST_DECRYPT = 3,
- REQUEST_CLOSE = 4 /* Request the mail to be closed with discard
- changes set to true */
} gpgol_wmsg_type;
typedef struct
commit 27c65853abdcccf7d739a7e2b09d7910641ac011
Author: Andre Heinecke <aheinecke at intevation.de>
Date: Mon Nov 28 14:59:48 2016 +0100
Don't close non-crypto mails
* src/mail.cpp (Mail::close_all_mails): Ignore non crypto mails.
Don't delete mail on error.
--
Closing non crypto mails is a chance for data loss as it could
have caused drafts to be discarded. The delete on error is also
not good as we don't know if the close already triggered the
unload event and so may have unloaded the mail already.
diff --git a/src/mail.cpp b/src/mail.cpp
index 1b2cb70..25d851a 100644
--- a/src/mail.cpp
+++ b/src/mail.cpp
@@ -861,6 +861,10 @@ Mail::close_all_mails ()
TRACEPOINT;
for (it = g_mail_map.begin(); it != g_mail_map.end(); ++it)
{
+ if (!it->second->is_crypto_mail())
+ {
+ continue;
+ }
if (it->second->close ())
{
log_error ("Failed to close mail: %p ", it->first);
@@ -870,10 +874,6 @@ Mail::close_all_mails ()
err++;
}
}
- else
- {
- delete it->second;
- }
}
return err;
}
commit c9775be375292e1f268937c099ed8bf9b994b771
Author: Andre Heinecke <aheinecke at intevation.de>
Date: Mon Nov 28 14:58:09 2016 +0100
Fix race between deletion and parser access
* src/mail.cpp (Mail::~Mail): Lock destruction.
(do_decrypt): Lock validity check and parser access.
--
This fixes a crash that could be triggered by wildly switching
between encrypted / signed mails.
diff --git a/src/mail.cpp b/src/mail.cpp
index 1db0d6f..1b2cb70 100644
--- a/src/mail.cpp
+++ b/src/mail.cpp
@@ -99,8 +99,16 @@ Mail::Mail (LPDISPATCH mailitem) :
g_mail_map.insert (std::pair<LPDISPATCH, Mail *> (mailitem, this));
}
+GPGRT_LOCK_DEFINE(dtor_lock);
+
Mail::~Mail()
{
+ /* This should fix a race condition where the mail is
+ deleted before the parser is accessed in the decrypt
+ thread. The shared_ptr of the parser then ensures
+ that the parser is alive even if the mail is deleted
+ while parsing. */
+ gpgrt_lock_lock (&dtor_lock);
std::map<LPDISPATCH, Mail *>::iterator it;
detach_MailItemEvents_sink (m_event_sink);
@@ -123,8 +131,17 @@ Mail::~Mail()
xfree (m_sender);
gpgol_release(m_mailitem);
- log_oom_extra ("%s:%s: destroyed: %p uuid: %s",
- SRCNAME, __func__, this, m_uuid.c_str());
+ if (!m_uuid.empty())
+ {
+ log_oom_extra ("%s:%s: destroyed: %p uuid: %s",
+ SRCNAME, __func__, this, m_uuid.c_str());
+ }
+ else
+ {
+ log_oom_extra ("%s:%s: non crypto mail: %p destroyed",
+ SRCNAME, __func__, this);
+ }
+ gpgrt_lock_unlock (&dtor_lock);
}
Mail *
@@ -448,32 +465,41 @@ GPGRT_LOCK_DEFINE(parser_lock);
static DWORD WINAPI
do_parsing (LPVOID arg)
{
- log_debug ("%s:%s: preparing the parser for: %p",
- SRCNAME, __func__, arg);
-
+ gpgrt_lock_lock (&dtor_lock);
+ /* We lock with mail dtors so we can be sure the mail->parser
+ call is valid. */
Mail *mail = (Mail *)arg;
- auto parser = mail->parser();
- if (!parser)
+ if (!Mail::is_valid_ptr (mail))
{
- log_error ("%s:%s: no parser found for mail: %p",
+ log_debug ("%s:%s: canceling parsing for: %p already deleted",
SRCNAME, __func__, arg);
- return -1;
+ gpgrt_lock_unlock (&dtor_lock);
+ return 0;
}
+ /* This takes a shared ptr of parser. So the parser is
+ still valid when the mail is deleted. */
+ auto parser = mail->parser();
+ gpgrt_lock_unlock (&dtor_lock);
+
gpgrt_lock_lock (&parser_lock);
- /* Serialize here to avoid too many
+ /* We lock the parser here to avoid too many
decryption attempts if there are
multiple mailobjects which might have already
- been deleted (e.g. by quick switches of the mailview. */
- if (Mail::is_valid_ptr (mail))
- {
- parser->parse();
- do_in_ui_thread (PARSING_DONE, arg);
- }
- else
+ been deleted (e.g. by quick switches of the mailview.)
+ Let's rather be a bit slower.
+ */
+ log_debug ("%s:%s: preparing the parser for: %p",
+ SRCNAME, __func__, arg);
+
+ if (!parser)
{
- log_debug ("%s:%s: canceling parsing for: %p already deleted",
+ log_error ("%s:%s: no parser found for mail: %p",
SRCNAME, __func__, arg);
+ gpgrt_lock_unlock (&parser_lock);
+ return -1;
}
+ parser->parse();
+ do_in_ui_thread (PARSING_DONE, arg);
gpgrt_lock_unlock (&parser_lock);
return 0;
}
-----------------------------------------------------------------------
Summary of changes:
src/mail.cpp | 116 ++++++++++++++++++++++++++----------------
src/mail.h | 17 +++++--
src/mailitem-events.cpp | 130 +++++++-----------------------------------------
src/windowmessages.cpp | 59 ----------------------
src/windowmessages.h | 3 --
5 files changed, 104 insertions(+), 221 deletions(-)
hooks/post-receive
--
GnuPG extension for MS Outlook
http://git.gnupg.org
More information about the Gnupg-commits
mailing list