Mercurial > pidgin.yaz

changeset 20666:0e1bc5c51030

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
A few leak plugs, some unnecessary allocation prevention, null pointer deref fixes and general robustness fixes. I'm sure there is more to do here, I got tired... Fixes #3308
author Daniel Atallah <daniel.atallah@gmail.com>
date Thu, 27 Sep 2007 04:41:04 +0000
parents 601083413e36
children cdabb4ee9468
files libpurple/protocols/msn/contact.c
diffstat 1 files changed, 173 insertions(+), 194 deletions(-) [+]
line wrap: on
line diff
--- a/libpurple/protocols/msn/contact.c	Wed Sep 26 21:03:16 2007 +0000
+++ b/libpurple/protocols/msn/contact.c	Thu Sep 27 04:41:04 2007 +0000
@@ -31,8 +31,8 @@
 
 const char *MsnSoapPartnerScenarioText[] =
 {
-        "Initial",
-        "ContactSave",
+	"Initial",
+	"ContactSave",
 	"MessengerPendingList",
 	"ContactMsgrAPI",
 	"BlockUnblock"
@@ -72,32 +72,23 @@
 msn_callback_state_new(void)
 {
 	return g_new0(MsnCallbackState, 1);
-}	
+}
 
 void
 msn_callback_state_free(MsnCallbackState *state)
 {
 	if (state == NULL)
 		return;
-	
-	if (state->who != NULL)
-		g_free(state->who);
-	
-	if (state->uid != NULL)
-		g_free(state->uid);
 
-	if (state->old_group_name != NULL)
-		g_free(state->old_group_name);
-	
-	if (state->new_group_name != NULL)
-		g_free(state->new_group_name);
-	
-	if (state->guid != NULL)
-		g_free(state->guid);
+	g_free(state->who);
+	g_free(state->uid);
+	g_free(state->old_group_name);
+	g_free(state->new_group_name);
+	g_free(state->guid);
 
 	g_free(state);
 }
-	
+
 void
 msn_callback_state_set_who(MsnCallbackState *state, const gchar *who)
 {
@@ -107,27 +98,23 @@
 
 	if (who != NULL)
 		new_str = g_strdup(who);
-	
-	if (state->who != NULL)
-		g_free(state->who);
-	
+
+	g_free(state->who);
 	state->who = new_str;
 }
 
 void
 msn_callback_state_set_uid(MsnCallbackState *state, const gchar *uid)
 {
-        gchar *new_str = NULL;
+	gchar *new_str = NULL;
 
-        g_return_if_fail(state != NULL);
+	g_return_if_fail(state != NULL);
 
-        if (uid != NULL)
-                new_str = g_strdup(uid);
+	if (uid != NULL)
+		new_str = g_strdup(uid);
 
-        if (state->uid != NULL)
-                g_free(state->uid);
-
-        state->uid = new_str;
+	g_free(state->uid);
+	state->uid = new_str;
 }
 
 void
@@ -140,9 +127,7 @@
 	if (old_group_name != NULL)
 		new_str = g_strdup(old_group_name);
 
-	if (state->old_group_name != NULL)
-		g_free(state->old_group_name);
-	
+	g_free(state->old_group_name);
 	state->old_group_name = new_str;
 }
 
@@ -156,9 +141,7 @@
 	if (new_group_name != NULL)
 		new_str = g_strdup(new_group_name);
 
-	if (state->new_group_name != NULL)
-		g_free(state->new_group_name);
-	
+	g_free(state->new_group_name);
 	state->new_group_name = new_str;
 }
 
@@ -171,10 +154,8 @@
 
 	if (guid != NULL)
 		new_str = g_strdup(guid);
-	
-	if (state->guid != NULL)
-		g_free(state->guid);
-	
+
+	g_free(state->guid);
 	state->guid = new_str;
 }
 
@@ -183,7 +164,7 @@
 msn_callback_state_set_list_id(MsnCallbackState *state, MsnListId list_id)
 {
 	g_return_if_fail(state != NULL);
-	
+
 	state->list_id = list_id;
 }
 
@@ -191,10 +172,10 @@
 msn_callback_state_set_action(MsnCallbackState *state, MsnCallbackAction action)
 {
 	g_return_if_fail(state != NULL);
-	
+
 	state->action |= action;
 }
-		       
+
 /*contact SOAP server login error*/
 static void
 msn_contact_login_error_cb(MsnSoapConn *soapconn, PurpleSslConnection *gsc, PurpleSslErrorType error)
@@ -227,9 +208,10 @@
 
 /*get MSN member role utility*/
 static MsnListId
-msn_get_memberrole(char * role)
+msn_get_memberrole(char *role)
 {
-	
+	g_return_val_if_fail(role != NULL, 0);
+
 	if (!strcmp(role,"Allow")) {
 		return MSN_LIST_AL;
 	} else if (!strcmp(role,"Block")) {
@@ -243,9 +225,11 @@
 }
 
 /*get User Type*/
-static int 
-msn_get_user_type(char * type)
+static int
+msn_get_user_type(char *type)
 {
+	g_return_val_if_fail(type != NULL, 0);
+
 	if (!strcmp(type,"Regular")) {
 		return MSN_USER_TYPE_PASSPORT;
 	}
@@ -327,9 +311,7 @@
 	xmlnode *fault, *faultstringnode, *faultdetail, *errorcode;
 	xmlnode *node, *body, *response, *result, *services;
 	xmlnode *service, *memberships, *info, *handle, *handletype;
-	xmlnode *LastChangeNode;
 	xmlnode *membershipnode, *members, *member, *passportNode;
-	char *LastChangeStr;
 
 	session = contact->session;
 	node = xmlnode_from_str(contact->soapconn->body, contact->soapconn->body_len);
@@ -341,8 +323,12 @@
 
 	purple_debug_misc("MSNCL","Parsing contact list with size %d\n", contact->soapconn->body_len);
 
-	purple_debug_misc("MSNCL","Root node @ %p: Name: '%s', child: '%s', lastchild: '%s'\n",node,node->name,node->child->name,node->lastchild->name);
-	body = xmlnode_get_child(node,"Body");
+	purple_debug_misc("MSNCL","Root node @ %p: Name: '%s', child: '%s', lastchild: '%s'\n", node,
+		node->name ? node->name : "(null)",
+		(node->child && node->child->name) ? node->child->name : "(null)",
+		(node->lastchild && node->lastchild->name) ? node->lastchild->name : "(null)");
+
+	body = xmlnode_get_child(node, "Body");
 
 	if (body == NULL) {
 		purple_debug_warning("MSNCL", "Failed to parse contact list Body node\n");
@@ -357,20 +343,20 @@
 
 		if ( (faultstringnode = xmlnode_get_child(fault, "faultstring")) != NULL ) {
 			gchar * faultstring = xmlnode_get_data(faultstringnode);
-			purple_debug_info("MSNCL","Faultstring: %s\n", faultstring);
+			purple_debug_info("MSNCL", "Faultstring: %s\n", faultstring ? faultstring : "(null)");
 			g_free(faultstring);
 		}
 		if ( (faultdetail = xmlnode_get_child(fault, "detail")) != NULL ) {
 			purple_debug_info("MSNCL","detail @ %p, name: %s\n",faultdetail, faultdetail->name);
 
 			if ( (errorcode = xmlnode_get_child(faultdetail, "errorcode")) != NULL ) {
-				purple_debug_info("MSNCL","errorcode @ %p, name: %s\n",errorcode, errorcode->name);
+				purple_debug_info("MSNCL","errorcode @ %p, name: %s\n", errorcode, errorcode->name);
 
 				if (errorcode->child != NULL) {
 					gchar *errorcodestring = xmlnode_get_data(errorcode);
-					purple_debug_info("MSNCL", "Error Code: %s\n", errorcodestring);
+					purple_debug_info("MSNCL", "Error Code: %s\n", errorcodestring ? errorcodestring : "(null)");
 
-					if ( !strncmp(errorcodestring, "ABDoesNotExist", 14) ) {
+					if (errorcodestring && !strncmp(errorcodestring, "ABDoesNotExist", 14) ) {
 						xmlnode_free(node);
 						g_free(errorcodestring);
 						msn_create_address_book(contact);
@@ -418,7 +404,7 @@
 	for (service = xmlnode_get_child(services, "Service"); service;
 	                                service = xmlnode_get_next_twin(service)) {
 		purple_debug_info("MSNCL","Service @ %p\n",service);
-	
+
 		if ( (info = xmlnode_get_child(service,"Info")) == NULL ) {
 			purple_debug_error("MSNCL","Error getting 'Info' child node\n");
 			continue;
@@ -434,7 +420,6 @@
 
 		if ( (typedata = xmlnode_get_data(handletype)) == NULL) {
 			purple_debug_error("MSNCL","Error retrieving data from 'Type' child node\n");
-			g_free(typedata);
 			continue;
 		}
 
@@ -447,11 +432,13 @@
 		}
 
 		if ( !g_strcasecmp(typedata, "Messenger") ) {
+			char *LastChangeStr = NULL;
+			xmlnode *LastChangeNode;
 
 			/*Last Change Node*/
-			LastChangeNode = xmlnode_get_child(service, "LastChange");
-			LastChangeStr = xmlnode_get_data(LastChangeNode);
-			purple_debug_info("MSNCL","LastChangeNode: '%s'\n",LastChangeStr);	
+			if ((LastChangeNode = xmlnode_get_child(service, "LastChange")))
+				LastChangeStr = xmlnode_get_data(LastChangeNode);
+			purple_debug_info("MSNCL","LastChangeNode: '%s'\n",LastChangeStr ? LastChangeStr : "(null)");
 			purple_account_set_string(session->account, "CLLastChange", LastChangeStr);
 			g_free(LastChangeStr);
 
@@ -466,35 +453,44 @@
 			for (membershipnode = xmlnode_get_child(memberships, "Membership"); membershipnode;
 							membershipnode = xmlnode_get_next_twin(membershipnode)){
 				xmlnode *roleNode;
-				char *role;
+				char *role = NULL;
+				list = 0;
 
-				roleNode = xmlnode_get_child(membershipnode,"MemberRole");
-				role = xmlnode_get_data(roleNode);
-				list = msn_get_memberrole(role);
+				if ((roleNode = xmlnode_get_child(membershipnode,"MemberRole"))) {
+					role = xmlnode_get_data(roleNode);
+					list = msn_get_memberrole(role);
+				}
 				list_op = 1 << list;
 
-				purple_debug_info("MSNCL","MemberRole role: %s, list_op: %d\n",role,list_op);
-				
+				purple_debug_info("MSNCL","MemberRole role: %s, list_op: %d\n", role ? role : "(null)", list_op);
+
 				g_free(role);
-				
-				members = xmlnode_get_child(membershipnode,"Members");
+
+				members = xmlnode_get_child(membershipnode, "Members");
 				for (member = xmlnode_get_child(members, "Member"); member;
 						member = xmlnode_get_next_twin(member)){
 					MsnUser *user = NULL;
-					xmlnode *typeNode, *membershipIdNode=NULL;
+					xmlnode *typeNode, *membershipIdNode = NULL;
 					gchar *type, *membershipId = NULL;
+					const char *member_type = xmlnode_get_attrib(member, "type");
+
+					purple_debug_info("MSNCL","Member type: %s\n", member_type ? member_type : "(null)");
+
+					if (!member_type)
+						continue;
 
-					purple_debug_info("MSNCL","Member type: %s\n", xmlnode_get_attrib(member,"type"));
-					
-					if( !g_strcasecmp(xmlnode_get_attrib(member,"type"), "PassportMember") ) {
-						passportNode = xmlnode_get_child(member,"PassportName");
-						passport = xmlnode_get_data(passportNode);
-						typeNode = xmlnode_get_child(member,"Type");
-						type = xmlnode_get_data(typeNode);
-						purple_debug_info("MSNCL","Passport name: '%s', Type: %s\n",passport,type);
+					if(!g_strcasecmp(member_type, "PassportMember") ) {
+						passport = type = NULL;
+						if ((passportNode = xmlnode_get_child(member, "PassportName")))
+							passport = xmlnode_get_data(passportNode);
+						if ((typeNode = xmlnode_get_child(member, "Type")))
+							type = xmlnode_get_data(typeNode);
+						purple_debug_info("MSNCL","Passport name: '%s', Type: %s\n", passport ? passport : "(null)", type ? type : "(null)");
+						/* Why do we even bother parsing it just to free it??? */
 						g_free(type);
 
 						user = msn_userlist_find_add_user(session->userlist,passport,NULL);
+						g_free(passport);
 
 						membershipIdNode = xmlnode_get_child(member,"MembershipId");
 						if (membershipIdNode != NULL) {
@@ -504,23 +500,22 @@
 								g_free(membershipId);
 							}
 						}
-							
+
 						msn_got_lst_user(session, user, list_op, NULL);
-
-						g_free(passport);
 					}
-					
-					if (!g_strcasecmp(xmlnode_get_attrib(member,"type"),"PhoneMember")) {
+					else if (!g_strcasecmp(member_type, "PhoneMember")) {
 					}
-					
-					if (!g_strcasecmp(xmlnode_get_attrib(member,"type"),"EmailMember")) {
+					else if (!g_strcasecmp(member_type, "EmailMember")) {
 						xmlnode *emailNode;
+						passport = NULL;
 
-						emailNode = xmlnode_get_child(member,"Email");
-						passport = xmlnode_get_data(emailNode);
-						purple_debug_info("MSNCL","Email Member: Name: '%s', list_op: %d\n", passport, list_op);
+						if ((emailNode = xmlnode_get_child(member, "Email")))
+							passport = xmlnode_get_data(emailNode);
+						purple_debug_info("MSNCL","Email Member: Name: '%s', list_op: %d\n", passport ? passport : "(null)", list_op);
+
 						user = msn_userlist_find_add_user(session->userlist, passport, NULL);
-						
+						g_free(passport);
+
 						membershipIdNode = xmlnode_get_child(member,"MembershipId");
 						if (membershipIdNode != NULL) {
 							membershipId = xmlnode_get_data(membershipIdNode);
@@ -529,9 +524,8 @@
 								g_free(membershipId);
 							}
 						}
-						
+
 						msn_got_lst_user(session, user, list_op, NULL);
-						g_free(passport);
 					}
 				}
 			}
@@ -670,34 +664,40 @@
 {
 	MsnSession *session = contact->session;
 	xmlnode *contactNode;
+	char *passport = NULL, *Name = NULL, *uid = NULL, *type = NULL;
 
 	for(contactNode = xmlnode_get_child(node, "Contact"); contactNode;
-				contactNode = xmlnode_get_next_twin(contactNode)){
-		xmlnode *contactId,*contactInfo,*contactType,*passportName,*displayName,*guid;
-		xmlnode *groupIds;
+				contactNode = xmlnode_get_next_twin(contactNode)) {
+		xmlnode *contactId, *contactInfo, *contactType, *passportName, *displayName, *guid, *groupIds;
 		MsnUser *user;
 		MsnUserType usertype;
-		char *passport = NULL, *Name = NULL, *uid = NULL, *type = NULL;
+
+		if (!(contactId = xmlnode_get_child(contactNode,"contactId"))
+				|| !(contactInfo = xmlnode_get_child(contactNode, "contactInfo"))
+				|| !(contactType = xmlnode_get_child(contactInfo, "contactType")))
+			continue;
 
-		contactId= xmlnode_get_child(contactNode,"contactId");
+		g_free(passport);
+		g_free(Name);
+		g_free(uid);
+		g_free(type);
+		passport = Name = uid = type = NULL;
+
 		uid = xmlnode_get_data(contactId);
-
-		contactInfo = xmlnode_get_child(contactNode,"contactInfo");
-		contactType = xmlnode_get_child(contactInfo,"contactType");
 		type = xmlnode_get_data(contactType);
 
 		/*setup the Display Name*/
-		if (!strcmp(type, "Me")){
-			char *friendly;
-			friendly = xmlnode_get_data(xmlnode_get_child(contactInfo, "displayName"));
-			purple_connection_set_display_name(session->account->gc, purple_url_decode(friendly));
+		if (type && !strcmp(type, "Me")){
+			char *friendly = NULL;
+			if ((displayName = xmlnode_get_child(contactInfo, "displayName")))
+				friendly = xmlnode_get_data(displayName);
+			purple_connection_set_display_name(session->account->gc, friendly ? purple_url_decode(friendly) : NULL);
 			g_free(friendly);
-			g_free(uid);
-			g_free(type);
 			continue; /* Not adding own account as buddy to buddylist */
 		}
+
 		usertype = msn_get_user_type(type);
-		passportName = xmlnode_get_child(contactInfo,"passportName");
+		passportName = xmlnode_get_child(contactInfo, "passportName");
 		if (passportName == NULL) {
 			xmlnode *emailsNode, *contactEmailNode, *emailNode;
 			xmlnode *messengerEnabledNode;
@@ -705,78 +705,65 @@
 
 			/*TODO: add it to the none-instant Messenger group and recognize as email Membership*/
 			/*Yahoo User?*/
-			emailsNode = xmlnode_get_child(contactInfo,"emails");
+			emailsNode = xmlnode_get_child(contactInfo, "emails");
 			if (emailsNode == NULL) {
 				/*TODO:  need to support the Mobile type*/
-				g_free(uid);
-				g_free(type);
 				continue;
 			}
-			for(contactEmailNode = xmlnode_get_child(emailsNode,"ContactEmail");contactEmailNode;
+			for(contactEmailNode = xmlnode_get_child(emailsNode, "ContactEmail"); contactEmailNode;
 					contactEmailNode = xmlnode_get_next_twin(contactEmailNode) ){
-				messengerEnabledNode = xmlnode_get_child(contactEmailNode,"isMessengerEnabled");
-				if(messengerEnabledNode == NULL){
-					g_free(uid);
-					g_free(type);
+				if (!(messengerEnabledNode = xmlnode_get_child(contactEmailNode, "isMessengerEnabled"))) {
+					/* XXX: Should this be a continue instead of a break? It seems like it'd cause unpredictable results otherwise. */
 					break;
 				}
+
 				msnEnabled = xmlnode_get_data(messengerEnabledNode);
-				if(!strcmp(msnEnabled,"true")){
-					/*Messenger enabled, Get the Passport*/
-					emailNode = xmlnode_get_child(contactEmailNode,"email");
+
+				if ((emailNode = xmlnode_get_child(contactEmailNode, "email"))) {
+					g_free(passport);
 					passport = xmlnode_get_data(emailNode);
-					purple_debug_info("MsnAB","Yahoo User %s\n",passport);
+				}
+
+				if(msnEnabled && !strcmp(msnEnabled, "true")) {
+					/*Messenger enabled, Get the Passport*/
+					purple_debug_info("MsnAB", "Yahoo User %s\n", passport ? passport : "(null)");
 					usertype = MSN_USER_TYPE_YAHOO;
-					g_free(uid);
-					g_free(type);
-					g_free(passport);
 					g_free(msnEnabled);
 					break;
-				}else{
+				} else {
 					/*TODO maybe we can just ignore it in Purple?*/
-					emailNode = xmlnode_get_child(contactEmailNode,"email");
-					passport = xmlnode_get_data(emailNode);
-					purple_debug_info("MSNAB","Other type user\n");
+					purple_debug_info("MSNAB", "Other type user\n");
 				}
+
 				g_free(msnEnabled);
 			}
 		} else {
 			passport = xmlnode_get_data(passportName);
 		}
 
-		if (passport == NULL) {
-			g_free(uid);
-			g_free(type);
+		if (passport == NULL)
 			continue;
-		}
 
-		displayName = xmlnode_get_child(contactInfo,"displayName");
-		if (displayName == NULL) {
+		if ((displayName = xmlnode_get_child(contactInfo, "displayName")))
+			Name = xmlnode_get_data(displayName);
+		else
 			Name = g_strdup(passport);
-		} else {
-			Name = xmlnode_get_data(displayName);
-		}
 
 		purple_debug_misc("MsnAB","passport:{%s} uid:{%s} display:{%s}\n",
-						passport,uid,Name);
+						passport, uid ? uid : "(null)", Name ? Name : "(null)");
 
-		user = msn_userlist_find_add_user(session->userlist, passport,Name);
-		msn_user_set_uid(user,uid);
+		user = msn_userlist_find_add_user(session->userlist, passport, Name);
+		msn_user_set_uid(user, uid);
 		msn_user_set_type(user, usertype);
-		g_free(Name);
-		g_free(passport);
-		g_free(uid);
-		g_free(type);
 
 		purple_debug_misc("MsnAB","parse guid...\n");
-		groupIds = xmlnode_get_child(contactInfo,"groupIds");
+		groupIds = xmlnode_get_child(contactInfo, "groupIds");
 		if (groupIds) {
-			for (guid = xmlnode_get_child(groupIds, "guid");guid;
+			for (guid = xmlnode_get_child(groupIds, "guid"); guid;
 							guid = xmlnode_get_next_twin(guid)){
-				char *group_id;
-				group_id = xmlnode_get_data(guid);
-				msn_user_add_group_id(user,group_id);
-				purple_debug_misc("MsnAB","guid:%s\n",group_id);
+				char *group_id = xmlnode_get_data(guid);
+				msn_user_add_group_id(user, group_id);
+				purple_debug_misc("MsnAB", "guid:%s\n", group_id ? group_id : "(null)");
 				g_free(group_id);
 			}
 		} else {
@@ -786,6 +773,11 @@
 
 		msn_got_lst_user(session, user, MSN_LIST_FL_OP, NULL);
 	}
+
+	g_free(passport);
+	g_free(Name);
+	g_free(uid);
+	g_free(type);
 }
 
 static gboolean
@@ -814,13 +806,14 @@
 	
 	body = xmlnode_get_child(node,"Body");
 	purple_debug_misc("MSN AddressBook","body{%p},name:%s\n",body,body->name);
-	
+
+	/* TODO: This appears to be used in a number of places and should be de-duplicated */
 	if ( (fault = xmlnode_get_child(body, "Fault")) != NULL) {
 		purple_debug_info("MSN AddressBook","Fault received from SOAP server!\n");
 		
 		if ( (faultstringnode = xmlnode_get_child(fault, "faultstring")) != NULL ) {
 			gchar *faultstring = xmlnode_get_data(faultstringnode);
-			purple_debug_info("MSN AddressBook","Faultstring: %s\n", faultstring);
+			purple_debug_info("MSN AddressBook","Faultstring: %s\n", faultstring ? faultstring : "(null)");
 			g_free(faultstring);
 		}
 		if ( (faultdetail = xmlnode_get_child(fault, "detail")) != NULL ) {
@@ -831,9 +824,9 @@
 				purple_debug_info("MSN AddressBook","errorcode @ %p, name: %s\n",errorcode, errorcode->name);
 
 				errorcodestring = xmlnode_get_data(errorcode);
-				purple_debug_info("MSN AddressBook", "Error Code: %s\n", errorcodestring);
+				purple_debug_info("MSN AddressBook", "Error Code: %s\n", errorcodestring ? errorcodestring : "(null)");
 						
-				if ( !strncmp(errorcodestring, "ABDoesNotExist", 14) ) {
+				if (errorcodestring && !strncmp(errorcodestring, "ABDoesNotExist", 14) ) {
 					g_free(errorcodestring);
 					xmlnode_free(node);
 					return TRUE;
@@ -895,20 +888,20 @@
 
 	abNode =xmlnode_get_child(result,"ab");
 	if(abNode != NULL){
-		xmlnode *LastChangeNode, *DynamicItemLastChangedNode;
-		char *lastchange, *dynamicChange;
+		xmlnode *node2;
+		char *tmp = NULL;
 
-		LastChangeNode = xmlnode_get_child(abNode,"lastChange");
-		lastchange = xmlnode_get_data(LastChangeNode);
-		purple_debug_info("MsnAB"," lastchanged Time:{%s}\n",lastchange);
-		purple_account_set_string(session->account, "ablastChange", lastchange);
+		if ((node2 = xmlnode_get_child(abNode, "lastChange")))
+			tmp = xmlnode_get_data(node2);
+		purple_debug_info("MsnAB"," lastchanged Time:{%s}\n", tmp ? tmp : "(null)");
+		purple_account_set_string(session->account, "ablastChange", tmp);
 
-		DynamicItemLastChangedNode = xmlnode_get_child(abNode,"DynamicItemLastChanged");
-		dynamicChange = xmlnode_get_data(DynamicItemLastChangedNode);
-		purple_debug_info("MsnAB"," DynamicItemLastChanged :{%s}\n",dynamicChange);
-		purple_account_set_string(session->account, "DynamicItemLastChanged", lastchange);
-		g_free(dynamicChange);
-		g_free(lastchange);
+		g_free(tmp); tmp = NULL;
+		if ((node2 = xmlnode_get_child(abNode, "DynamicItemLastChanged")))
+			tmp = xmlnode_get_data(node2);
+		purple_debug_info("MsnAB"," DynamicItemLastChanged :{%s}\n", tmp ? tmp : "(null)");
+		purple_account_set_string(session->account, "DynamicItemLastChanged", tmp);
+		g_free(tmp);
 	}
 
 	xmlnode_free(node);
@@ -969,27 +962,19 @@
 msn_get_address_book(MsnContact *contact, const MsnSoapPartnerScenario partner_scenario, const char *LastChanged, const char *dynamicItemLastChange)
 {
 	MsnSoapReq *soap_request;
-	char *body = NULL;
-	char *ab_update_str,*update_str;
+	char *body;
+	char *update_str = NULL;
 
 	purple_debug_misc("MSN AddressBook","Getting Address Book\n");
 
 	/*build SOAP and POST it*/
-	if ( LastChanged != NULL ) {
-		ab_update_str = g_strdup_printf(MSN_GET_ADDRESS_UPDATE_XML,LastChanged);
-	} else {
-		ab_update_str = g_strdup("");
-	}
-	if ( dynamicItemLastChange != NULL ) {
-		update_str = g_strdup_printf(MSN_GET_ADDRESS_UPDATE_XML,
-									 dynamicItemLastChange);
-	} else {
-		update_str = g_strdup(ab_update_str);
-	}
-	g_free(ab_update_str);
-	
+	if (dynamicItemLastChange != NULL)
+		update_str = g_strdup_printf(MSN_GET_ADDRESS_UPDATE_XML, dynamicItemLastChange);
+	else if (LastChanged != NULL)
+		update_str = g_strdup_printf(MSN_GET_ADDRESS_UPDATE_XML, LastChanged);
 
-	body = g_strdup_printf(MSN_GET_ADDRESS_TEMPLATE, MsnSoapPartnerScenarioText[partner_scenario], update_str);
+
+	body = g_strdup_printf(MSN_GET_ADDRESS_TEMPLATE, MsnSoapPartnerScenarioText[partner_scenario], update_str ? update_str : "");
 	g_free(update_str);
 
 	soap_request = msn_soap_request_new(MSN_CONTACT_SERVER,
@@ -1063,7 +1048,8 @@
 	MsnSoapReq *soap_request;
 	gchar *body = NULL;
 	gchar *contact_xml = NULL;
-	gchar *soap_action;
+
+	g_return_if_fail(passport != NULL);
 /*	gchar *escaped_displayname;
 
 
@@ -1084,11 +1070,10 @@
 	g_free(contact_xml);
 
 	/*build SOAP and POST it*/
-	soap_action = g_strdup(MSN_CONTACT_ADD_SOAP_ACTION);
 
 	soap_request = msn_soap_request_new(MSN_CONTACT_SERVER,
 					MSN_ADDRESS_BOOK_POST_URL,
-					soap_action,
+					MSN_CONTACT_ADD_SOAP_ACTION,
 					body,
 					state,
 					msn_add_contact_read_cb,
@@ -1096,7 +1081,6 @@
 					msn_contact_connect_init);
 	msn_soap_post(contact->soapconn,soap_request);
 
-	g_free(soap_action);
 	g_free(body);
 }
 
@@ -1165,7 +1149,7 @@
 	MsnSoapReq *soap_request;
 	MsnUserList *userlist;
 	MsnUser *user;
-	gchar *body = NULL, *soap_action, *contact_xml;
+	gchar *body = NULL, *contact_xml;
 
 	g_return_if_fail(passport != NULL);
 	g_return_if_fail(groupId != NULL);
@@ -1214,11 +1198,10 @@
 	g_free(contact_xml);
 
 	/*build SOAP and POST it*/
-	soap_action = g_strdup(MSN_ADD_CONTACT_GROUP_SOAP_ACTION);
 
 	soap_request = msn_soap_request_new(MSN_CONTACT_SERVER,
 					MSN_ADDRESS_BOOK_POST_URL,
-					soap_action,
+					MSN_ADD_CONTACT_GROUP_SOAP_ACTION,
 					body,
 					state,
 					msn_add_contact_to_group_read_cb,
@@ -1226,7 +1209,6 @@
 					msn_contact_connect_init);
 	msn_soap_post(contact->soapconn,soap_request);
 
-	g_free(soap_action);
 	g_free(body);
 }
 
@@ -1340,7 +1322,7 @@
 	MsnUserList * userlist;
 	MsnUser *user;
 	MsnCallbackState *state;
-	gchar *body = NULL, *soap_action, *contact_id_xml;
+	gchar *body = NULL, *contact_id_xml;
 	const gchar *groupId;
 	
 	g_return_if_fail(passport != NULL);
@@ -1375,25 +1357,22 @@
 	msn_callback_state_set_who(state, passport);
 	msn_callback_state_set_guid(state, groupId);
 	msn_callback_state_set_old_group_name(state, group_name);
-		
+
 	contact_id_xml = g_strdup_printf(MSN_CONTACT_ID_XML, user->uid);
 	body = g_strdup_printf(MSN_CONTACT_DEL_GROUP_TEMPLATE, contact_id_xml, groupId);
 	g_free(contact_id_xml);
-	
+
 	/*build SOAP and POST it*/
-	soap_action = g_strdup(MSN_CONTACT_DEL_GROUP_SOAP_ACTION);
-	
 	soap_request = msn_soap_request_new(MSN_CONTACT_SERVER,
 					    MSN_ADDRESS_BOOK_POST_URL,
-					    soap_action,
+					    MSN_CONTACT_DEL_GROUP_SOAP_ACTION,
 					    body,
 					    state,
 					    msn_del_contact_from_group_read_cb,
 					    msn_del_contact_from_group_written_cb,
 					    msn_contact_connect_init);
 	msn_soap_post(contact->soapconn,soap_request);
-	
-	g_free(soap_action);
+
 	g_free(body);
 }