Merge branch 'jb52265' into 'master'

Improve user change and vpnd crash reply error processing

See merge request mer-core/connman!303

connman/src/storage.c

@@ -2148,7 +2148,7 @@ static void change_user_reply(DBusPendingCall *call, void *user_data)
 	DBusMessage *reply;
 	DBusError error;
 	int err = 0;
-	int delay = USER_CHANGE_DELAY;
+	unsigned int delay = USER_CHANGE_DELAY;
 
 	data = user_data;
 
@@ -2213,18 +2213,26 @@ static void change_user_reply(DBusPendingCall *call, void *user_data)
 	case -EBUSY:
 		/* D-Bus was congested, double delay */
 		delay += USER_CHANGE_DELAY;
-	case -ETIMEDOUT:
+
 		/* fall through */
+	case -ETIMEDOUT:
 	case -ENOTCONN:
-		/* fall through */
 	case -ENONET:
 		if (!init_delayed_user_change(data, delay)) {
 			send_vpnd_change_user = true;
 			goto out_no_free;
 		}
 
-		/* Otherwise, fall though */
+		/*
+		 * Delayed user change already in progress. Reply the result if
+		 * changed via D-Bus, don't revert user.
+		 */
+		goto delayed;
 	default:
+		/*
+		 * Invalid user, permission denied or any other error reverts
+		 * to root user.
+		 */
 		goto err;
 	}
 
@@ -2264,14 +2272,15 @@ static void change_user_reply(DBusPendingCall *call, void *user_data)
 		storage_change_uid(data->uid);
 	}
 
+delayed:
 	if (!data->pending)
 		goto out;
 
-	switch (-err) {
-	case EALREADY:
+	switch (err) {
+	case -EALREADY:
 		reply = __connman_error_already_enabled(data->pending);
 		break;
-	case ENOENT:
+	case -ENOENT:
 		reply = __connman_error_not_found(data->pending);
 		break;
 	default:
@@ -2604,18 +2613,31 @@ int __connman_storage_change_user(uid_t uid,
 	return err;
 }
 
+/*
+ * This gets called after processing the reply. In case there was a delayed
+ * ongoing user change in progress the real error, EBUSY, ETIMEDOUT, ENOTCONN
+ * or ENONET is returned which do not require further actions.
+ */
 static void result_cb(uid_t uid, int err, void *user_data)
 {
-	if (err && err != -EALREADY) {
+	switch (err) {
+	case 0:
+	case -EALREADY:
+		DBG("user %u changed to vpnd", uid);
+		break;
+	case -EBUSY:
+	case -ETIMEDOUT:
+	case -ENOTCONN:
+	case -ENONET:
+		DBG("user change to %u is in progress", uid);
+		break;
+	default:
 		connman_error("changing uid %u to vpnd failed (err: %d), "
-				"reset to uid %u", storage.current_uid, err,
-				geteuid());
+				"reset to uid %u", uid, err, geteuid());
 		set_user_dir(NULL, STORAGE_DIR_TYPE_MAIN, false);
 		storage_change_uid(geteuid());
-		return;
+		break;
 	}
-
-	DBG("user %u changed to vpnd", storage.current_uid);
 }
 
 static void vpnd_created(DBusConnection *conn, void *user_data)

connman/unit/test-storage.c

@@ -3483,16 +3483,38 @@ static void storage_test_vpn_crash1()
 	g_free(test_path);
 }
 
-/* Send user change, then crash vpnd */
+struct vpn_error_data {
+	const char *errorname;
+	uid_t uid;
+	int events;
+};
+
+/*
+ * Send user change, then crash vpnd. Check with different errors that the
+ * user is not changed in wrong cases back to root. Also test that all actions
+ * will result in a delayed user change, which will get a faked OK reply.
+ */
 static void storage_test_vpn_crash2()
 {
+	struct vpn_error_data vpn_data[] = {
+			/* These 4 will initiate delayed user change */
+			{"net.connman.Error.Timeout", UID_USER, 1},
+			{"net.connman.Error.NotConnected", UID_USER, 1},
+			{"net.connman.Error.NoReply", UID_USER, 1},
+			{"net.connman.Error.UnknownMethod", UID_USER, 1},
+			/* No new call is to be made */
+			{"net.connman.Error.AlreadyEnabled",UID_USER, 0},
+			/* An "other" error, caused by fs access error etc. */
+			{"net.connman.Error.Failed", UID_ROOT, 0},
+	};
 	GMainLoop *mainloop;
 	gchar *test_path;
 	mode_t m_dir = 0700;
 	mode_t m_file = 0600;
 	DBusConnection *connection;
 	DBusMessage *reply;
 	DBusError error;
+	unsigned int i;
 
 	mainloop = g_main_loop_new(NULL, FALSE);
 	test_path = setup_test_directory();
@@ -3526,33 +3548,52 @@ static void storage_test_vpn_crash2()
 	clean_dbus();
 	init_dbus(TRUE);
 
-	/* vpnd crashed */
-	disconnect_watch(connection, NULL);
+	for (i = 0; i < G_N_ELEMENTS(vpn_data); i++) {
+		DBG("iterate %d", i);
 
-	/* vpnd started, send_change_user_msg() is used */
-	connect_watch(connection, NULL);
+		/* vpnd crashed */
+		disconnect_watch(connection, NULL);
 
-	/* Fake error as reply, failed can result from any of mkdir errors. */
-	reply = create_dbus_error(last_message, "net.connman.Error.Failed");
-	g_assert(reply);
-	set_reply_error(reply);
+		/* vpnd started, send_change_user_msg() is used */
+		connect_watch(connection, NULL);
 
-	/* Call the pending callback */
-	DBG("call connmand pending call notify");
-	last_pending_function(last_pending_call, last_pending_function_data);
+		/* Fake error as reply */
+		reply = create_dbus_error(last_message, vpn_data[i].errorname);
+		g_assert(reply);
+		set_reply_error(reply);
 
-	/* User is reset to root in case the error is unknown */
-	g_assert_cmpint(current_uid, ==, UID_ROOT);
+		/* Call the pending callback */
+		DBG("call connmand pending call notify");
+		last_pending_function(last_pending_call,
+						last_pending_function_data);
 
-	/* No events in main loop either */
-	g_assert_cmpint(iterate_main_context(g_main_loop_get_context(mainloop),
-				FALSE, 1), ==, 0);
+		/* User is reset to root in case the error is unknown */
+		g_assert_cmpint(current_uid, ==, vpn_data[i].uid);
 
-	g_assert(last_reply_error);
-	g_assert_null(last_reply);
+		/* Different errors result in different amount of events */
+		g_assert_cmpint(iterate_main_context(
+					g_main_loop_get_context(mainloop),
+					FALSE, 1), ==, vpn_data[i].events);
 
-	clean_dbus();
-	init_dbus(TRUE);
+		g_assert(last_reply_error);
+		g_assert_null(last_reply);
+
+		if (vpn_data[i].events) {
+			/* Fake reply */
+			reply = g_dbus_create_reply(last_message,
+							DBUS_TYPE_INVALID);
+			g_assert(reply);
+			set_reply(reply);
+
+			/* Call the pending callback to handle event */
+			DBG("call connmand pending call notify");
+			last_pending_function(last_pending_call,
+						last_pending_function_data);
+		}
+
+		clean_dbus();
+		init_dbus(TRUE);
+	}
 
 	/* vpnd crashed for second time */
 	disconnect_watch(connection, NULL);