[git] GpgOL - branch, master, updated. gpgol-2.2.0-5-g9f132ff
by Andre Heinecke
cvs at cvs.gnupg.org
Wed Jun 20 14:03:36 CEST 2018
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 9f132ff7cbc2f2484ce234f7d4bb163bb743f703 (commit)
via e26e6b58b48e45c5e5ce9a1ac3d2e0e28a4cb277 (commit)
from 8576131bc84d41b8b68ae92461c063774d3064fc (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 9f132ff7cbc2f2484ce234f7d4bb163bb743f703
Author: Andre Heinecke <aheinecke at intevation.de>
Date: Wed Jun 20 13:56:19 2018 +0200
Prepare keycache for autoencryption
* src/keycache.cpp,
* src/mail.cpp, src/mail.h (set_crypto_selected_manually),
(increment_locate_count, decrement_locate_count): New stateholders.
* src/keycache.cpp (LocateArgs): Argument helper to carry mail.
(startLocate, startLocateSecret): Add mail argument and use it.
* src/keycache.h: Update accordingly.
--
This keeps track of how many locate threads are running
on a mail. So that we can handle the state where
all locates for a mail are done.
This prepares:
GnuPG-Bug-Id: T3999
diff --git a/src/keycache.cpp b/src/keycache.cpp
index 3239e33..277f461 100644
--- a/src/keycache.cpp
+++ b/src/keycache.cpp
@@ -23,6 +23,7 @@
#include "common.h"
#include "cpphelp.h"
+#include "mail.h"
#include <gpg-error.h>
#include <gpgme++/context.h>
@@ -35,6 +36,53 @@
GPGRT_LOCK_DEFINE (keycache_lock);
static KeyCache* singleton = nullptr;
+/** At some point we need to set a limit. There
+ seems to be no limit on how many recipients a mail
+ can have in outlook.
+
+ We would run out of resources or block.
+
+ 50 Threads already seems a bit excessive but
+ it should really cover most legit use cases.
+*/
+
+#define MAX_LOCATOR_THREADS 50
+static int s_thread_cnt;
+
+namespace
+{
+ class LocateArgs
+ {
+ public:
+ LocateArgs (const std::string& mbox, Mail *mail = nullptr):
+ m_mbox (mbox),
+ m_mail (mail)
+ {
+ s_thread_cnt++;
+ Mail::delete_lock ();
+ if (Mail::is_valid_ptr (m_mail))
+ {
+ m_mail->increment_locate_count ();
+ }
+ Mail::delete_unlock ();
+ };
+
+ ~LocateArgs()
+ {
+ s_thread_cnt--;
+ Mail::delete_lock ();
+ if (Mail::is_valid_ptr (m_mail))
+ {
+ m_mail->decrement_locate_count ();
+ }
+ Mail::delete_unlock ();
+ }
+
+ std::string m_mbox;
+ Mail *m_mail;
+ };
+} // namespace
+
class KeyCache::Private
{
public:
@@ -471,7 +519,7 @@ do_locate_secret (LPVOID arg)
}
void
-KeyCache::startLocate (char **recipients) const
+KeyCache::startLocate (char **recipients, Mail *mail) const
{
if (!recipients)
{
@@ -480,12 +528,12 @@ KeyCache::startLocate (char **recipients) const
}
for (int i = 0; recipients[i]; i++)
{
- startLocate (recipients[i]);
+ startLocate (recipients[i], mail);
}
}
void
-KeyCache::startLocate (const char *addr) const
+KeyCache::startLocate (const char *addr, Mail *mail) const
{
if (!addr)
{
@@ -505,8 +553,9 @@ KeyCache::startLocate (const char *addr) const
d->m_pgp_key_map.insert (std::pair<std::string, GpgME::Key> (recp, GpgME::Key()));
log_debug ("%s:%s Creating a locator thread",
SRCNAME, __func__);
+ const auto args = new LocateArgs(recp, mail);
HANDLE thread = CreateThread (NULL, 0, do_locate,
- (LPVOID) strdup (recp.c_str ()), 0,
+ args, 0,
NULL);
CloseHandle (thread);
}
@@ -514,7 +563,7 @@ KeyCache::startLocate (const char *addr) const
}
void
-KeyCache::startLocateSecret (const char *addr) const
+KeyCache::startLocateSecret (const char *addr, Mail *mail) const
{
if (!addr)
{
@@ -534,8 +583,10 @@ KeyCache::startLocateSecret (const char *addr) const
d->m_pgp_skey_map.insert (std::pair<std::string, GpgME::Key> (recp, GpgME::Key()));
log_debug ("%s:%s Creating a locator thread",
SRCNAME, __func__);
+ const auto args = new LocateArgs(recp, mail);
+
HANDLE thread = CreateThread (NULL, 0, do_locate_secret,
- (LPVOID) strdup (recp.c_str ()), 0,
+ (LPVOID) args, 0,
NULL);
CloseHandle (thread);
}
diff --git a/src/keycache.h b/src/keycache.h
index 6bda0b9..0364ab1 100644
--- a/src/keycache.h
+++ b/src/keycache.h
@@ -34,6 +34,8 @@ namespace GpgME
class Key;
};
+class Mail;
+
class KeyCache
{
protected:
@@ -58,15 +60,19 @@ public:
/* Start a key location in a background thread filling
the key cache. cArray is a null terminated array
- of address strings. */
- void startLocate (char **cArray) const;
+ of address strings.
+
+ The mail argument is used to add / remove the
+ locator thread counter.
+ */
+ void startLocate (char **cArray, Mail *mail) const;
/* Look for a secret key for the addr. */
- void startLocateSecret (const char *addr) const;
+ void startLocateSecret (const char *addr, Mail *mail) const;
/* Start a key location in a background thread filling
the key cache. */
- void startLocate (const char *addr) const;
+ void startLocate (const char *addr, Mail *mail) const;
// Internal for thread
void setSmimeKey(const std::string &mbox, const GpgME::Key &key);
diff --git a/src/mail.cpp b/src/mail.cpp
index 64d14e5..d61d22b 100644
--- a/src/mail.cpp
+++ b/src/mail.cpp
@@ -97,7 +97,9 @@ Mail::Mail (LPDISPATCH mailitem) :
m_is_forwarded_crypto_mail(false),
m_is_reply_crypto_mail(false),
m_is_send_again(false),
- m_disable_att_remove_warning(false)
+ m_disable_att_remove_warning(false),
+ m_manual_crypto_opts(false),
+ m_locate_count(0)
{
if (get_mail_for_item (mailitem))
{
@@ -2674,9 +2676,9 @@ Mail::locate_keys()
// First update oom data to have recipients and sender updated.
update_oom_data ();
char ** recipients = take_cached_recipients ();
- KeyCache::instance()->startLocateSecret (get_sender ().c_str ());
- KeyCache::instance()->startLocate (get_sender ().c_str ());
- KeyCache::instance()->startLocate (recipients);
+ KeyCache::instance()->startLocateSecret (get_sender ().c_str (), this);
+ KeyCache::instance()->startLocate (get_sender ().c_str (), this);
+ KeyCache::instance()->startLocate (recipients, this);
release_cArray (recipients);
locate_in_progress = false;
}
@@ -3219,3 +3221,15 @@ Mail::set_block_html(bool value)
{
m_block_html = value;
}
+
+void
+Mail::increment_locate_count()
+{
+ m_locate_count++;
+}
+
+void
+Mail::decrement_locate_count()
+{
+ m_locate_count--;
+}
diff --git a/src/mail.h b/src/mail.h
index 83483f2..7e6ea21 100644
--- a/src/mail.h
+++ b/src/mail.h
@@ -530,6 +530,18 @@ public:
/* Remove automatic loading of HTML references setting. */
void set_block_status ();
+ /* Crypto options (sign/encrypt) have been set manually. */
+ void set_crypto_selected_manually (bool v) { m_manual_crypto_opts = v; }
+ // bool is_crypto_selected_manually () const { return m_manual_crypto_opts; }
+
+ /* Reference that a resolver thread is running for this mail. */
+ void increment_locate_count ();
+
+ /* To be called when a resolver thread is done. If there are no running
+ resolver threads we can check the recipients to see if we should
+ toggle / untoggle the secure state. */
+ void decrement_locate_count ();
+
private:
void update_categories ();
void update_sigstate ();
@@ -574,5 +586,7 @@ private:
bool m_is_send_again; /* Is this a send again of a crypto mail */
bool m_disable_att_remove_warning; /* Should not warn about attachment removal. */
bool m_block_html; /* Force blocking of html content. e.g for unsigned S/MIME mails. */
+ bool m_manual_crypto_opts; /* Crypto options (sign/encrypt) have been set manually. */
+ int m_locate_count; /* The number of key locates pending for this mail. */
};
#endif // MAIL_H
commit e26e6b58b48e45c5e5ce9a1ac3d2e0e28a4cb277
Author: Andre Heinecke <aheinecke at intevation.de>
Date: Wed Jun 20 13:33:28 2018 +0200
Locks and more locks
* src/mail.cpp (mail_map_lock, uid_map_lock): New locks.
(Mail::Mail, Mail::~Mail, Mail::get_mail_for_item),
(Mail::get_mail_for_uuid, Mail::is_valid_ptr),
(Mail::close_all_mails, Mail::revert_all_mails),
(Mail::revert_all_mails, Mail::wipe_all_mails),
(Mail::set_uuid, Mail::locate_all_crypto_recipients)
(Mail::delete_lock, Mail::delete_unlock): New helpers to
ensure a mail pointer stays valid.
* src/mail.h: Update accordingly.
--
As the mail object is used from more and more threads
and will soon be used even more for autoencryption
we have to be _very_ careful to minimize / remove race
conditions which could lead to a crash.
For autoencryption we will have async code working
on every new mail.
Prepares:
GnuPG-Bug-Id: T3999
diff --git a/src/mail.cpp b/src/mail.cpp
index 47d7367..64d14e5 100644
--- a/src/mail.cpp
+++ b/src/mail.cpp
@@ -63,6 +63,9 @@ static std::map<LPDISPATCH, Mail*> s_mail_map;
static std::map<std::string, Mail*> s_uid_map;
static std::set<std::string> uids_searched;
+GPGRT_LOCK_DEFINE (mail_map_lock);
+GPGRT_LOCK_DEFINE (uid_map_lock);
+
static Mail *s_last_mail;
#define COPYBUFSIZE (8 * 1024)
@@ -113,12 +116,28 @@ Mail::Mail (LPDISPATCH mailitem) :
gpgol_release(mailitem);
return;
}
+ gpgrt_lock_lock (&mail_map_lock);
s_mail_map.insert (std::pair<LPDISPATCH, Mail *> (mailitem, this));
+ gpgrt_lock_unlock (&mail_map_lock);
s_last_mail = this;
}
GPGRT_LOCK_DEFINE(dtor_lock);
+// static
+void
+Mail::delete_lock ()
+{
+ gpgrt_lock_lock (&dtor_lock);
+}
+
+// static
+void
+Mail::delete_unlock ()
+{
+ gpgrt_lock_unlock (&dtor_lock);
+}
+
Mail::~Mail()
{
/* This should fix a race condition where the mail is
@@ -138,19 +157,23 @@ Mail::~Mail()
log_oom_extra ("%s:%s: Erasing mail",
SRCNAME, __func__);
+ gpgrt_lock_lock (&mail_map_lock);
it = s_mail_map.find(m_mailitem);
if (it != s_mail_map.end())
{
s_mail_map.erase (it);
}
+ gpgrt_lock_unlock (&mail_map_lock);
if (!m_uuid.empty())
{
+ gpgrt_lock_lock (&uid_map_lock);
auto it2 = s_uid_map.find(m_uuid);
if (it2 != s_uid_map.end())
{
s_uid_map.erase (it2);
}
+ gpgrt_lock_unlock (&uid_map_lock);
}
log_oom_extra ("%s:%s: releasing mailitem",
@@ -186,7 +209,9 @@ Mail::get_mail_for_item (LPDISPATCH mailitem)
return NULL;
}
std::map<LPDISPATCH, Mail *>::iterator it;
+ gpgrt_lock_lock (&mail_map_lock);
it = s_mail_map.find(mailitem);
+ gpgrt_lock_unlock (&mail_map_lock);
if (it == s_mail_map.end())
{
return NULL;
@@ -201,7 +226,9 @@ Mail::get_mail_for_uuid (const char *uuid)
{
return NULL;
}
+ gpgrt_lock_lock (&uid_map_lock);
auto it = s_uid_map.find(std::string(uuid));
+ gpgrt_lock_unlock (&uid_map_lock);
if (it == s_uid_map.end())
{
return NULL;
@@ -212,13 +239,18 @@ Mail::get_mail_for_uuid (const char *uuid)
bool
Mail::is_valid_ptr (const Mail *mail)
{
+ gpgrt_lock_lock (&mail_map_lock);
auto it = s_mail_map.begin();
while (it != s_mail_map.end())
{
if (it->second == mail)
- return true;
+ {
+ gpgrt_lock_unlock (&mail_map_lock);
+ return true;
+ }
++it;
}
+ gpgrt_lock_unlock (&mail_map_lock);
return false;
}
@@ -1553,7 +1585,9 @@ Mail::close_all_mails ()
int err = 0;
std::map<LPDISPATCH, Mail *>::iterator it;
TRACEPOINT;
+ gpgrt_lock_lock (&mail_map_lock);
std::map<LPDISPATCH, Mail *> mail_map_copy = s_mail_map;
+ gpgrt_lock_unlock (&mail_map_lock);
for (it = mail_map_copy.begin(); it != mail_map_copy.end(); ++it)
{
/* XXX For non racy code the is_valid_ptr check should not
@@ -1587,6 +1621,7 @@ Mail::revert_all_mails ()
{
int err = 0;
std::map<LPDISPATCH, Mail *>::iterator it;
+ gpgrt_lock_lock (&mail_map_lock);
for (it = s_mail_map.begin(); it != s_mail_map.end(); ++it)
{
if (it->second->revert ())
@@ -1604,6 +1639,7 @@ Mail::revert_all_mails ()
continue;
}
}
+ gpgrt_lock_unlock (&mail_map_lock);
return err;
}
@@ -1612,6 +1648,7 @@ Mail::wipe_all_mails ()
{
int err = 0;
std::map<LPDISPATCH, Mail *>::iterator it;
+ gpgrt_lock_lock (&mail_map_lock);
for (it = s_mail_map.begin(); it != s_mail_map.end(); ++it)
{
if (it->second->wipe ())
@@ -1620,6 +1657,7 @@ Mail::wipe_all_mails ()
err++;
}
}
+ gpgrt_lock_unlock (&mail_map_lock);
return err;
}
@@ -2094,7 +2132,10 @@ Mail::set_uuid()
SRCNAME, __func__, m_mailitem, uuid);
delete other;
}
+
+ gpgrt_lock_lock (&uid_map_lock);
s_uid_map.insert (std::pair<std::string, Mail *> (m_uuid, this));
+ gpgrt_lock_unlock (&uid_map_lock);
log_debug ("%s:%s: uuid for %p is now %s",
SRCNAME, __func__, this,
m_uuid.c_str());
@@ -2966,6 +3007,7 @@ Mail::locate_all_crypto_recipients()
return;
}
+ gpgrt_lock_lock (&mail_map_lock);
std::map<LPDISPATCH, Mail *>::iterator it;
for (it = s_mail_map.begin(); it != s_mail_map.end(); ++it)
{
@@ -2974,6 +3016,7 @@ Mail::locate_all_crypto_recipients()
it->second->locate_keys ();
}
}
+ gpgrt_lock_unlock (&mail_map_lock);
}
int
diff --git a/src/mail.h b/src/mail.h
index 7f4ed7b..83483f2 100644
--- a/src/mail.h
+++ b/src/mail.h
@@ -92,8 +92,34 @@ public:
*/
static Mail* get_last_mail ();
+ /** @brief voids the last mail variable. */
static void invalidate_last_mail ();
+ /** @brief Lock mail deletion.
+
+ Mails are heavily accessed multi threaded. E.g. when locating
+ keys. Due to bad timing it would be possible that between
+ a check for "is_valid_ptr" to see if a map is still valid
+ and the usage of the mail a delete would happen.
+
+ This lock can be used to prevent that. Changes made to the
+ mail will of course have no effect as the mail is already in
+ the process of beeing unloaded. And calls that access MAPI
+ or OOM still might crash. But this at least gurantees that
+ the member variables of the mail exist while the lock is taken.
+
+ Use it in your thread like:
+
+ Mail::delete_lock ();
+ Mail::is_valid_ptr (mail);
+ mail->set_or_check_something ();
+ Mail::delete_unlock ();
+
+ Still be carefull when it is a mapi or oom function.
+ */
+ static void delete_lock ();
+ static void delete_unlock ();
+
/** @brief looks for existing Mail objects.
@returns A reference to an existing mailitem or NULL in case none
-----------------------------------------------------------------------
Summary of changes:
src/keycache.cpp | 63 +++++++++++++++++++++++++++++++++++++++++++++++-----
src/keycache.h | 14 ++++++++----
src/mail.cpp | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
src/mail.h | 40 +++++++++++++++++++++++++++++++++
4 files changed, 169 insertions(+), 15 deletions(-)
hooks/post-receive
--
GnuPG extension for MS Outlook
http://git.gnupg.org
More information about the Gnupg-commits
mailing list