ENGINE-250: Pretty sure the current bugs are now actual bugs and not sql-related. But will check. insert-or-replace for identity-based set functions is now replaced with a slightly convoluted callback mechanism which does a check for existence and then updates or inserts appropriately, with optional transaction guards (to be set to false if they're called in what's already a transaction, e.g. set_identity)

5 years ago · d528a5bfa8
parent e90ff8be26
commit d528a5bfa8
3 changed files with 174 additions and 76 deletions
--- a/src/pEpEngine.c
+++ b/src/pEpEngine.c
@ -11,6 +11,8 @@
 #include <stdlib.h>
 #include <sqlite3.h>

+#define _PEP_SQLITE_DEBUG 0
+
 static volatile int init_count = -1;

 // sql overloaded functions - modified from sqlite3.c
@ -38,6 +40,7 @@ static void _sql_lower(sqlite3_context* ctx, int argc, sqlite3_value** argv) {
    }
 }

+#if _PEP_SQLITE_DEBUG
 int sql_trace_callback (unsigned trace_constant, 
                        void* context_ptr,
                        void* P,
@ -63,7 +66,7 @@ int sql_trace_callback (unsigned trace_constant,
    }
    return 0;
 }
-
+#endif

 // sql manipulation statements
 static const char *sql_log = 
@ -195,6 +198,10 @@ static const char *sql_set_pgp_keypair =
    "insert or ignore into pgp_keypair (fpr) "
    "values (upper(replace(?1,' ',''))) ;";

+static const char* sql_exists_identity_entry = 
+    "select count(*) from identity "
+    "   where address = ?1 and user_id = ?2;";
+
 static const char *sql_set_identity_entry = 
    "insert into identity ("
    "       address, main_key_id, "
@ -242,6 +249,10 @@ static const char *sql_set_trust =
 static const char *sql_update_trust =
    "update trust set comm_type = ?3 " 
    "   where user_id = ?1 and pgp_keypair_fpr = upper(replace(?2,' ',''));";
+
+static const char* sql_exists_trust_entry = 
+    "select count(*) from trust "
+    "   where user_id = ?1 and pgp_keypair_fpr = upper(replace(?2,' ',''));";
    
 static const char *sql_update_trust_for_fpr =
    "update trust "
@ -499,7 +510,9 @@ DYNAMIC_API PEP_STATUS init(PEP_SESSION *session)
        goto pep_error;
    }
    
+#if _PEP_SQLITE_DEBUG    
    sqlite3_config(SQLITE_CONFIG_LOG, errorLogCallback, NULL);
+#endif

    int_result = sqlite3_open_v2(
            LOCAL_DB,
@ -528,10 +541,12 @@ DYNAMIC_API PEP_STATUS init(PEP_SESSION *session)

    sqlite3_busy_timeout(_session->db, BUSY_WAIT_TIME);

+#if _PEP_SQLITE_DEBUG
    sqlite3_trace_v2(_session->db, 
        SQLITE_TRACE_STMT | SQLITE_TRACE_ROW | SQLITE_TRACE_CLOSE,
        sql_trace_callback,
        NULL);
+#endif

    assert(SYSTEM_DB);
    if (SYSTEM_DB == NULL) {
@ -1018,6 +1033,10 @@ DYNAMIC_API PEP_STATUS init(PEP_SESSION *session)
            (int)strlen(sql_update_person), &_session->update_person, NULL);
    assert(int_result == SQLITE_OK);

+    int_result = sqlite3_prepare_v2(_session->db, sql_exists_person,
+            (int)strlen(sql_exists_person), &_session->exists_person, NULL);
+    assert(int_result == SQLITE_OK);
+
    int_result = sqlite3_prepare_v2(_session->db, sql_set_as_pep_user,
            (int)strlen(sql_set_as_pep_user), &_session->set_as_pep_user, NULL);
    assert(int_result == SQLITE_OK);
@ -1026,10 +1045,6 @@ DYNAMIC_API PEP_STATUS init(PEP_SESSION *session)
            (int)strlen(sql_is_pep_user), &_session->is_pep_user, NULL);
    assert(int_result == SQLITE_OK);

-    int_result = sqlite3_prepare_v2(_session->db, sql_exists_person,
-            (int)strlen(sql_exists_person), &_session->exists_person, NULL);
-    assert(int_result == SQLITE_OK);
-
    int_result = sqlite3_prepare_v2(_session->db, sql_set_device_group,
            (int)strlen(sql_set_device_group), &_session->set_device_group, NULL);
    assert(int_result == SQLITE_OK);
@ -1051,6 +1066,10 @@ DYNAMIC_API PEP_STATUS init(PEP_SESSION *session)
            (int)strlen(sql_update_identity_entry), &_session->update_identity_entry, NULL);
    assert(int_result == SQLITE_OK);

+    int_result = sqlite3_prepare_v2(_session->db, sql_exists_identity_entry,
+            (int)strlen(sql_exists_identity_entry), &_session->exists_identity_entry, NULL);
+    assert(int_result == SQLITE_OK);
+
    int_result = sqlite3_prepare_v2(_session->db, sql_set_identity_flags,
            (int)strlen(sql_set_identity_flags), &_session->set_identity_flags,
            NULL);
@ -1069,6 +1088,10 @@ DYNAMIC_API PEP_STATUS init(PEP_SESSION *session)
            (int)strlen(sql_update_trust), &_session->update_trust, NULL);
    assert(int_result == SQLITE_OK);

+    int_result = sqlite3_prepare_v2(_session->db, sql_exists_trust_entry,
+                 (int)strlen(sql_exists_trust_entry), &_session->exists_trust_entry, NULL);
+    assert(int_result == SQLITE_OK);
+
    int_result = sqlite3_prepare_v2(_session->db, sql_update_trust_for_fpr,
            (int)strlen(sql_update_trust_for_fpr), &_session->update_trust_for_fpr, NULL);
    assert(int_result == SQLITE_OK);
@ -1288,6 +1311,8 @@ DYNAMIC_API void release(PEP_SESSION session)
                sqlite3_finalize(session->get_device_group);
            if (session->set_pgp_keypair)
                sqlite3_finalize(session->set_pgp_keypair);
+            if (session->exists_identity_entry)
+                sqlite3_finalize(session->exists_identity_entry);                
            if (session->set_identity_entry)
                sqlite3_finalize(session->set_identity_entry);
            if (session->update_identity_entry)
@ -1296,6 +1321,8 @@ DYNAMIC_API void release(PEP_SESSION session)
                sqlite3_finalize(session->set_identity_flags);
            if (session->unset_identity_flags)
                sqlite3_finalize(session->unset_identity_flags);
+            if (session->exists_trust_entry)
+                sqlite3_finalize(session->exists_trust_entry);                                
            if (session->set_trust)
                sqlite3_finalize(session->set_trust);
            if (session->update_trust)
@ -1985,6 +2012,68 @@ PEP_STATUS get_identities_by_address(
    return PEP_STATUS_OK;
 }

+PEP_STATUS exists_identity_entry(PEP_SESSION session, pEp_identity* identity,
+                                 bool* exists) {
+    assert(session);
+    assert(identity);
+    assert(!EMPTYSTR(identity->user_id));        
+    assert(!EMPTYSTR(identity->address));
+    if (!session || !exists || EMPTYSTR(identity->user_id) || EMPTYSTR(identity->address))
+        return PEP_ILLEGAL_VALUE;
+    
+    *exists = false;
+    
+    sqlite3_reset(session->exists_identity_entry);
+    sqlite3_bind_text(session->exists_identity_entry, 1, identity->address, -1,
+                      SQLITE_STATIC);
+    sqlite3_bind_text(session->exists_identity_entry, 2, identity->user_id, -1,
+                      SQLITE_STATIC);
+                  
+    int result = sqlite3_step(session->exists_identity_entry);
+    switch (result) {
+        case SQLITE_ROW: {
+            // yeah yeah, I know, we could be lazy here, but it looks bad.
+            *exists = (sqlite3_column_int(session->exists_identity_entry, 0) != 0);
+            break;
+        }
+        default:
+            return PEP_UNKNOWN_ERROR;
+    }
+
+    return PEP_STATUS_OK;
+}
+
+PEP_STATUS exists_trust_entry(PEP_SESSION session, pEp_identity* identity,
+                              bool* exists) {
+    assert(session);
+    assert(identity);
+    assert(!EMPTYSTR(identity->user_id));        
+    assert(!EMPTYSTR(identity->fpr));
+    if (!session || !exists || EMPTYSTR(identity->user_id) || EMPTYSTR(identity->fpr))
+        return PEP_ILLEGAL_VALUE;
+    
+    *exists = false;
+    
+    sqlite3_reset(session->exists_trust_entry);
+    sqlite3_bind_text(session->exists_trust_entry, 1, identity->user_id, -1,
+                      SQLITE_STATIC);
+    sqlite3_bind_text(session->exists_trust_entry, 2, identity->fpr, -1,
+                      SQLITE_STATIC);
+                  
+    int result = sqlite3_step(session->exists_trust_entry);
+    switch (result) {
+        case SQLITE_ROW: {
+            // yeah yeah, I know, we could be lazy here, but it looks bad.
+            *exists = (sqlite3_column_int(session->exists_trust_entry, 0) != 0);
+            break;
+        }
+        default:
+            return PEP_UNKNOWN_ERROR;
+    }
+
+    return PEP_STATUS_OK;
+}
+
 // FIXME: We can rollback in set_identity on the return status,
 // so we should probably do that.
 PEP_STATUS set_pgp_keypair(PEP_SESSION session, const char* fpr) {
@ -2006,7 +2095,7 @@ PEP_STATUS set_pgp_keypair(PEP_SESSION session, const char* fpr) {
 }

 static PEP_STATUS _set_or_update_trust(PEP_SESSION session,
-                                       const pEp_identity* identity,
+                                       pEp_identity* identity,
                                       sqlite3_stmt* set_or_update) {

    assert(session);
@ -2035,7 +2124,7 @@ static PEP_STATUS _set_or_update_trust(PEP_SESSION session,
 }

 static PEP_STATUS _set_or_update_identity_entry(PEP_SESSION session,
-                                                const pEp_identity* identity,
+                                                pEp_identity* identity,
                                                sqlite3_stmt* set_or_update) {
    assert(session);
    assert(identity);
@ -2062,7 +2151,7 @@ static PEP_STATUS _set_or_update_identity_entry(PEP_SESSION session,
 }

 static PEP_STATUS _set_or_update_person(PEP_SESSION session, 
-                                        const pEp_identity* identity,
+                                        pEp_identity* identity,
                                        sqlite3_stmt* set_or_update) {
    assert(session);
    assert(identity);
@ -2093,62 +2182,64 @@ static PEP_STATUS _set_or_update_person(PEP_SESSION session,
 }

 PEP_STATUS set_or_update_with_identity(PEP_SESSION session,
-                                       const pEp_identity* identity,
-                                       PEP_STATUS (* set_function)(PEP_SESSION, const pEp_identity*, sqlite3_stmt*),
+                                       pEp_identity* identity,
+                                       PEP_STATUS (* set_function)(PEP_SESSION, pEp_identity*, sqlite3_stmt*),
+                                       PEP_STATUS (* exists_function)(PEP_SESSION, pEp_identity*, bool*),                                       
                                       sqlite3_stmt* update_query,
                                       sqlite3_stmt* set_query,
                                       bool guard_transaction) {

    if (guard_transaction) {
        sqlite3_exec(session->db, "BEGIN TRANSACTION ;", NULL, NULL, NULL);
-    }                      
-    PEP_STATUS status = set_function(session, identity, update_query);
-    if (status != PEP_STATUS_OK) {
-        if (guard_transaction)
-            sqlite3_exec(session->db, "ROLLBACK ;", NULL, NULL, NULL);
-        return status;    
-    }   
-    else {
-        if (sqlite3_changes(session->db) == 0) {
-            status = set_function(session, identity, set_query);            
-            if (status != PEP_STATUS_OK) {
-                if (guard_transaction)
-                     sqlite3_exec(session->db, "ROLLBACK ;", NULL, NULL, NULL);
-                return status;    
-            }                                  
-        }
    }
-    if (guard_transaction) {
-        sqlite3_exec(session->db, "COMMIT ;", NULL, NULL, NULL);
+    bool exists = false;
+    PEP_STATUS status = exists_function(session, identity, &exists);
+    
+    if (status == PEP_STATUS_OK) {
+        if (exists) {
+            status = set_function(session, identity, update_query);
+        }
+        else {
+            status = set_function(session, identity, set_query);                                              
+        }                    
+    }   
+    if (guard_transaction) {        
+        if (status != PEP_STATUS_OK)
+            sqlite3_exec(session->db, "ROLLBACK ;", NULL, NULL, NULL);
+        else 
+            sqlite3_exec(session->db, "COMMIT ;", NULL, NULL, NULL);
    }                      
    return status;
 }

-PEP_STATUS _set_trust_internal(PEP_SESSION session, const pEp_identity* identity,
+PEP_STATUS _set_trust_internal(PEP_SESSION session, pEp_identity* identity,
                               bool guard_transaction) {
    return set_or_update_with_identity(session, identity,
                                       _set_or_update_trust,
+                                       exists_trust_entry,
                                       session->update_trust,
                                       session->set_trust,
                                       guard_transaction);
 }
-PEP_STATUS set_trust(PEP_SESSION session, const pEp_identity* identity) {
+PEP_STATUS set_trust(PEP_SESSION session, pEp_identity* identity) {
    return _set_trust_internal(session, identity, true);
 }

-PEP_STATUS set_person(PEP_SESSION session, const pEp_identity* identity,
+PEP_STATUS set_person(PEP_SESSION session, pEp_identity* identity,
                      bool guard_transaction) {
    return set_or_update_with_identity(session, identity,
                                       _set_or_update_person,
+                                       exists_person,
                                       session->update_person,
                                       session->set_person,
                                       guard_transaction);
 }

-PEP_STATUS set_identity_entry(PEP_SESSION session, const pEp_identity* identity,
+PEP_STATUS set_identity_entry(PEP_SESSION session, pEp_identity* identity,
                              bool guard_transaction) {
    return set_or_update_with_identity(session, identity,
                                       _set_or_update_identity_entry,
+                                       exists_identity_entry,
                                       session->update_identity_entry,
                                       session->set_identity_entry,
                                       guard_transaction);
@ -2174,7 +2265,7 @@ DYNAMIC_API PEP_STATUS set_identity(
    
    bool listed;

-    bool has_fpr = (identity->fpr && identity->fpr[0] != '\0');
+    bool has_fpr = (!EMPTYSTR(identity->fpr));
    
    if (has_fpr) {    
        // blacklist check - FIXME: ENGINE-294 will remove
@ -2207,31 +2298,41 @@ DYNAMIC_API PEP_STATUS set_identity(
        }
    }

-    status = set_person(session, identity, false);
+    // We do this because there are checks in set_person for
+    // aliases, which modify the identity object on return.
+    pEp_identity* ident_copy = identity_dup(identity); 
+    if (!ident_copy)
+        return PEP_OUT_OF_MEMORY;
+
+    status = set_person(session, ident_copy, false);
    if (status != PEP_STATUS_OK) {
        sqlite3_exec(session->db, "ROLLBACK ;", NULL, NULL, NULL);
-        return status;
+        goto pep_free;
    }

-    status = set_identity_entry(session, identity, false);
+    status = set_identity_entry(session, ident_copy, false);
    if (status != PEP_STATUS_OK) {
        sqlite3_exec(session->db, "ROLLBACK ;", NULL, NULL, NULL);
-        return status;
+        goto pep_free;
    }

    if (has_fpr) {
-        status = _set_trust_internal(session, identity, false);
+        status = _set_trust_internal(session, ident_copy, false);
        if (status != PEP_STATUS_OK) {
            sqlite3_exec(session->db, "ROLLBACK ;", NULL, NULL, NULL);
-            return status;
+            goto pep_free;
        }
    }
    
    result = sqlite3_exec(session->db, "COMMIT ;", NULL, NULL, NULL);
    if (result == SQLITE_OK)
-        return PEP_STATUS_OK;
+        status = PEP_STATUS_OK;
    else
-        return PEP_COMMIT_FAILED;
+        status = PEP_COMMIT_FAILED;
+
+pep_free:
+    free_identity(ident_copy);
+    return status;
 }

 // This ONLY sets the user flag, and creates a shell identity if necessary.
@ -2241,19 +2342,15 @@ PEP_STATUS set_as_pep_user(PEP_SESSION session, pEp_identity* user) {
    assert(user);
    assert(user->address);
    assert(!EMPTYSTR(user->user_id));
-    
-    char* user_id = user->user_id;
-    
-    if (!session || !user || user->address || EMPTYSTR(user_id))
+        
+    if (!session || !user || user->address || EMPTYSTR(user->user_id))
        return PEP_ILLEGAL_VALUE;
            
    PEP_STATUS status = PEP_STATUS_OK;
    
-    char* alias_default = NULL;
-    
    bool person_exists = false;
    
-    status = exists_person(session, user_id, &alias_default, &person_exists);
+    status = exists_person(session, user, &person_exists);
    
    if (status != PEP_STATUS_OK)
        return status;
@ -2268,12 +2365,11 @@ PEP_STATUS set_as_pep_user(PEP_SESSION session, pEp_identity* user) {
        free_identity(tmp_id);
        if (status != PEP_STATUS_OK)
            return status;
-        alias_default = strdup(user->user_id);
    }
        
    // Ok, let's set it.
    sqlite3_reset(session->set_as_pep_user);
-    sqlite3_bind_text(session->set_as_pep_user, 1, alias_default, -1,
+    sqlite3_bind_text(session->set_as_pep_user, 1, user->user_id, -1,
            SQLITE_STATIC);
    int result = sqlite3_step(session->set_as_pep_user);
    sqlite3_reset(session->set_as_pep_user);
@ -2284,24 +2380,27 @@ PEP_STATUS set_as_pep_user(PEP_SESSION session, pEp_identity* user) {
    return PEP_STATUS_OK;    
 }

-PEP_STATUS exists_person(PEP_SESSION session, const char* user_id,
-                         char** default_id, bool* exists) {
+PEP_STATUS exists_person(PEP_SESSION session, pEp_identity* identity,
+                         bool* exists) {            
+    
+    // const char* user_id,
+    //                      char** default_id, bool* exists) {
    assert(session);
    assert(exists);
-    assert(!EMPTYSTR(user_id));
+    assert(identity);
+    assert(!EMPTYSTR(identity->user_id));
        
-    if (!session || !exists || EMPTYSTR(user_id))
+    if (!session || !exists || !identity || EMPTYSTR(identity->user_id))
        return PEP_ILLEGAL_VALUE;
    
    *exists = false;
-    
+
+    const char* user_id = identity->user_id;
    char* alias_default = NULL;
    
    PEP_STATUS status = get_userid_alias_default(session, user_id, &alias_default);
    
    if (status == PEP_CANNOT_FIND_ALIAS || EMPTYSTR(alias_default)) {
-        free(alias_default);
-        alias_default = NULL;
        sqlite3_reset(session->exists_person);
        sqlite3_bind_text(session->exists_person, 1, user_id, -1,
                SQLITE_STATIC);
@ -2310,23 +2409,23 @@ PEP_STATUS exists_person(PEP_SESSION session, const char* user_id,
            case SQLITE_ROW: {
                // yeah yeah, I know, we could be lazy here, but it looks bad.
                *exists = (sqlite3_column_int(session->exists_person, 0) != 0);
+                status = PEP_STATUS_OK;
                break;
            }
            default:
                return PEP_UNKNOWN_ERROR;
        }
-        if (*exists)
-            alias_default = strdup(user_id);
    }
-    else
+    else if (status == PEP_STATUS_OK) {
        *exists = true; // thank you, delete on cascade!
-
-    if (!default_id)
+        // FIXME: Should we correct the userid default here? I think we should.
+        free(identity->user_id);
+        identity->user_id = alias_default; // ownership transfer
+    }
+    else
        free(alias_default);
-    else    
-        *default_id = alias_default;
-    
-    return PEP_STATUS_OK;
+        
+    return status;
 }

 PEP_STATUS is_pep_user(PEP_SESSION session, pEp_identity *identity, bool* is_pep)
--- a/src/pEpEngine.h
+++ b/src/pEpEngine.h
@ -934,7 +934,7 @@ DYNAMIC_API PEP_STATUS get_trust(PEP_SESSION session, pEp_identity *identity);


 PEP_STATUS set_trust(PEP_SESSION session, 
-                     const pEp_identity* identity);
+                     pEp_identity* identity);
                            
 PEP_STATUS update_trust_for_fpr(PEP_SESSION session, 
                                const char* fpr, 
@ -1232,12 +1232,9 @@ PEP_STATUS import_trusted_own_keys(PEP_SESSION session);
 // This ONLY sets the *user* flag, and creates a shell identity if necessary.
 PEP_STATUS set_as_pep_user(PEP_SESSION session, pEp_identity* user);

-// returns true (by reference) if a person with this user_id exists; if default_id != NULL,
-// function will also return by reference a copy of the default id which
-// is this user_id is aliased to (which will usually, but not always, 
-// string equivalent to the user_id)
-PEP_STATUS exists_person(PEP_SESSION session, const char* user_id,
-                         char** default_id, bool* exists);
+// returns true (by reference) if a person with this user_id exists; 
+// Also replaces aliased user_ids by defaults in identity.
+PEP_STATUS exists_person(PEP_SESSION session, pEp_identity* identity, bool* exists);

 // returns true if the USER corresponding to this identity has been listed in
 // the *person* table as a pEp user. This *does not check comm_type*                         
--- a/src/pEp_internal.h
+++ b/src/pEp_internal.h
@ -131,19 +131,21 @@ struct _pEpSession {
    sqlite3_stmt *refresh_userid_default_key;
    sqlite3_stmt *remove_fpr_as_default;
    sqlite3_stmt *set_person;
-    sqlite3_stmt *update_person;    
+    sqlite3_stmt *update_person;
+    sqlite3_stmt *exists_person;    
    sqlite3_stmt *set_as_pep_user;
    sqlite3_stmt *is_pep_user;
-    sqlite3_stmt *exists_person;
    sqlite3_stmt *set_device_group;
    sqlite3_stmt *get_device_group;
    sqlite3_stmt *set_pgp_keypair;
    sqlite3_stmt *set_identity_entry;
-    sqlite3_stmt *update_identity_entry;    
+    sqlite3_stmt *update_identity_entry;
+    sqlite3_stmt *exists_identity_entry;        
    sqlite3_stmt *set_identity_flags;
    sqlite3_stmt *unset_identity_flags;
    sqlite3_stmt *set_trust;
    sqlite3_stmt *update_trust;    
+    sqlite3_stmt *exists_trust_entry;
    sqlite3_stmt *update_trust_for_fpr;
    sqlite3_stmt *get_trust;
    sqlite3_stmt *least_trust;