[PATCH 0/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[PATCH 0/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[jackie.huang]

From: Jackie Huang <jackie.huang@windriver.com>

A race condition can occur when adding users and groups to the
passwd and group files, in [YOCTO #1794], 10 times retry added
but it is not fixed completely.

This fix re-writes the useradd_preinst and useradd_sysroot with
python and use locking of bb.utils to lock the passwd and group
files before executing useradd/groupadd commands to avoid the
lock race themselves.

[YOCTO #2779]

Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
---
The following changes since commit 4bce3417917a3e88ba6529db394525fba82e0699:

  EFI: Make installer EFI aware (2012-07-19 17:48:53 +0100)

are available in the git repository at:
  git://git.pokylinux.org/poky-contrib jhuang0/yocto_2779_groupadd_0722
  http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=jhuang0/yocto_2779_groupadd_0722

Jackie Huang (1):
  useradd.bbclass: use locking of bb.utils to avoid lock race issue of
    useradd/groupadd

 meta/classes/useradd.bbclass |  284 ++++++++++++++++++------------------------
 1 files changed, 124 insertions(+), 160 deletions(-)

-- 
1.7.4

[PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[jackie.huang]

From: Jackie Huang <jackie.huang@windriver.com>

A race condition can occur when adding users and groups to the
passwd and group files, in [YOCTO #1794], 10 times retry added
but it is not fixed completely.

This fix re-writes the useradd_preinst and useradd_sysroot with
python and use locking of bb.utils to lock the passwd and group
files before executing useradd/groupadd commands to avoid the
lock race themselves.

[YOCTO #2779]

Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
---
 meta/classes/useradd.bbclass |  284 ++++++++++++++++++------------------------
 1 files changed, 124 insertions(+), 160 deletions(-)

diff --git a/meta/classes/useradd.bbclass b/meta/classes/useradd.bbclass
index bb8f42b..ed5ed69 100644
--- a/meta/classes/useradd.bbclass
+++ b/meta/classes/useradd.bbclass
@@ -14,126 +14,90 @@ USERADDDEPENDS_virtclass-nativesdk = ""
 # c) As the preinst script in the target package at do_rootfs time
 # d) As the preinst script in the target package on device as a package upgrade
 #
-useradd_preinst () {
-OPT=""
-SYSROOT=""
-
-if test "x$D" != "x"; then
-	# Installing into a sysroot
-	SYSROOT="$D"
-	OPT="--root $D"
-
-	# Add groups and users defined for all recipe packages
-	GROUPADD_PARAM="${@get_all_cmd_params(d, 'group')}"
-	USERADD_PARAM="${@get_all_cmd_params(d, 'user')}"
-else
-	# Installing onto a target
-	# Add groups and users defined only for this package
-	GROUPADD_PARAM="${GROUPADD_PARAM}"
-	USERADD_PARAM="${USERADD_PARAM}"
-fi
-
-# Perform group additions first, since user additions may depend
-# on these groups existing
-if test "x$GROUPADD_PARAM" != "x"; then
-	echo "Running groupadd commands..."
-	# Invoke multiple instances of groupadd for parameter lists
-	# separated by ';'
-	opts=`echo "$GROUPADD_PARAM" | cut -d ';' -f 1`
-	remaining=`echo "$GROUPADD_PARAM" | cut -d ';' -f 2-`
-	while test "x$opts" != "x"; do
-		groupname=`echo "$opts" | awk '{ print $NF }'`
-		group_exists=`grep "^$groupname:" $SYSROOT/etc/group || true`
-		if test "x$group_exists" = "x"; then
-			count=1
-			while true; do
-				eval $PSEUDO groupadd $OPT $opts || true
-				group_exists=`grep "^$groupname:" $SYSROOT/etc/group || true`
-				if test "x$group_exists" = "x"; then
-					# File locking issues can require us to retry the command
-					echo "WARNING: groupadd command did not succeed. Retrying..."
-					sleep 1
-				else
-					break
-				fi
-				count=`expr $count + 1`
-				if test $count = 11; then
-					echo "ERROR: tried running groupadd command 10 times without success, giving up"
-					exit 1
-				fi
-			done		
-		else
-			echo "Note: group $groupname already exists, not re-creating it"
-		fi
-
-		if test "x$opts" = "x$remaining"; then
-			break
-		fi
-		opts=`echo "$remaining" | cut -d ';' -f 1`
-		remaining=`echo "$remaining" | cut -d ';' -f 2-`
-	done
-fi 
-
-if test "x$USERADD_PARAM" != "x"; then
-	echo "Running useradd commands..."
-	# Invoke multiple instances of useradd for parameter lists
-	# separated by ';'
-	opts=`echo "$USERADD_PARAM" | cut -d ';' -f 1`
-	remaining=`echo "$USERADD_PARAM" | cut -d ';' -f 2-`
-	while test "x$opts" != "x"; do
-		# useradd does not have a -f option, so we have to check if the
-		# username already exists manually
-		username=`echo "$opts" | awk '{ print $NF }'`
-		user_exists=`grep "^$username:" $SYSROOT/etc/passwd || true`
-		if test "x$user_exists" = "x"; then
-			count=1
-			while true; do
-				eval $PSEUDO useradd $OPT $opts || true
-				user_exists=`grep "^$username:" $SYSROOT/etc/passwd || true`
-				if test "x$user_exists" = "x"; then
-					# File locking issues can require us to retry the command
-					echo "WARNING: useradd command did not succeed. Retrying..."
-					sleep 1
-				else
-					break
-				fi
-				count=`expr $count + 1`
-				if test $count = 11; then
-					echo "ERROR: tried running useradd command 10 times without success, giving up"
-					exit 1
-				fi
-			done
-		else
-			echo "Note: username $username already exists, not re-creating it"
-		fi
-
-		if test "x$opts" = "x$remaining"; then
-			break
-		fi
-		opts=`echo "$remaining" | cut -d ';' -f 1`
-		remaining=`echo "$remaining" | cut -d ';' -f 2-`
-	done
-fi
+def useradd_preinst(d):
+	import re
+
+	sysroot = ""
+	opt = ""
+
+	dir = d.getVar('STAGING_DIR_TARGET', True)
+	if dir:
+		# Installing into a sysroot
+		sysroot = dir
+		opt = "--root %s" % dir
+
+		# Add groups and users defined for all recipe packages
+		groupadd_param = get_all_cmd_params(d, 'group')
+		useradd_param = get_all_cmd_params(d, 'user')
+	else:
+		# Installing onto a target
+		# Add groups and users defined only for this package
+		groupadd_param = d.getVar('GROUPADD_PARAM', True)
+		useradd_param = d.getVar('GROUPADD_PARAM', True)
+
+	group_file = '%s/etc/group' % sysroot
+	user_file = '%s/etc/passwd' % sysroot
+
+	# Use the locking of bb to the group/passwd file to avoid the
+	# locking issue of groupadd/useradd
+	group_lock = '%s.locked' % group_file
+	user_lock = '%s.locked' % user_file
+	lockfiles = [group_lock, user_lock]
+
+	with bb.utils.fileslocked(lockfiles):
+		# Perform group additions first, since user additions may depend
+		# on these groups existing
+		if groupadd_param and sysroot:
+			bb.debug(1, "Running groupadd commands ...")
+			# Invoke multiple instances of groupadd for parameter lists
+			# separated by ';'
+			param_list = groupadd_param.split(';')
+			for opts in param_list:
+				groupname = opts.split()[-1]
+				with open(group_file, 'r') as f:
+					passwd_lines = f.read()
+				group_re = re.compile('\n%s' % groupname)
+				if group_re.search(passwd_lines):
+					bb.note("Note: groupname %s already exists, not re-creating it" % groupname)
+					continue
+				try:
+					output, err = bb.process.run('groupadd %s %s' % (opt, opts))
+				except bb.process.CmdError as exc:
+					bb.error("Failed to add group: %s" % exc)
+				else:
+					bb.note("Successful to add group %s" % groupname)
+
+		if useradd_param:
+			bb.debug(1, "Running useradd commands ...")
+			# Invoke multiple instances of useradd for parameter lists
+			# separated by ';'
+			param_list = useradd_param.split(';')
+			for opts in param_list:
+				# useradd does not have a -f option, so we have to check if the
+				# username already exists manually
+				username = opts.split()[-1]
+				with open(user_file, 'r') as f:
+					passwd_lines = f.read()
+				user_re = re.compile('\n%s' % username)
+				if user_re.search(passwd_lines):
+					bb.note("Note: username %s already exists, not re-creating it" % username)
+					continue
+				try:
+					output, err = bb.process.run('useradd %s %s' % (opt, opts))
+				except bb.process.CmdError as exc:
+					bb.error("Failed to add user: %s" % exc)
+				else:
+					bb.note("Successful to add user %s" % username)
+
+fakeroot python useradd_sysroot () {
+	useradd_preinst(d)
 }
 
-useradd_sysroot () {
-	# Pseudo may (do_install) or may not (do_populate_sysroot_setscene) be running 
-	# at this point so we're explicit about the environment so pseudo can load if 
-	# not already present.
-	export PSEUDO="${FAKEROOTENV} PSEUDO_LOCALSTATEDIR=${STAGING_DIR_TARGET}${localstatedir}/pseudo ${STAGING_DIR_NATIVE}${bindir}/pseudo"
-
-	# Explicitly set $D since it isn't set to anything
-	# before do_install
-	D=${STAGING_DIR_TARGET}
-	useradd_preinst
+fakeroot python useradd_sysroot_sstate () {
+    if d.getVar("BB_CURRENTTASK", True) == "package_setscene":
+        useradd_preinst(d)
 }
 
-useradd_sysroot_sstate () {
-	if [ "${BB_CURRENTTASK}" = "package_setscene" ]
-	then
-		useradd_sysroot
-	fi
-}
 
 do_install[prefuncs] += "${SYSROOTFUNC}"
 SYSROOTFUNC = "useradd_sysroot"
@@ -146,7 +110,7 @@ SYSROOTPOSTFUNC_virtclass-cross = ""
 SYSROOTPOSTFUNC_virtclass-native = ""
 SYSROOTPOSTFUNC_virtclass-nativesdk = ""
 
-USERADDSETSCENEDEPS = "${MLPREFIX}base-passwd:do_populate_sysroot_setscene shadow-native:do_populate_sysroot_setscene ${MLPREFIX}shadow-sysroot:do_populate_sysroot_setscene"
+USERADDSETSCENEDEPS = "base-passwd:do_populate_sysroot_setscene shadow-native:do_populate_sysroot_setscene ${MLPREFIX}shadow-sysroot:do_populate_sysroot_setscene"
 USERADDSETSCENEDEPS_virtclass-cross = ""
 USERADDSETSCENEDEPS_virtclass-native = ""
 USERADDSETSCENEDEPS_virtclass-nativesdk = ""
@@ -154,61 +118,61 @@ do_package_setscene[depends] = "${USERADDSETSCENEDEPS}"
 
 # Recipe parse-time sanity checks
 def update_useradd_after_parse(d):
-    useradd_packages = d.getVar('USERADD_PACKAGES', True)
+	useradd_packages = d.getVar('USERADD_PACKAGES', True)
 
-    if not useradd_packages:
-        raise bb.build.FuncFailed, "%s inherits useradd but doesn't set USERADD_PACKAGES" % d.getVar('FILE')
+	if not useradd_packages:
+		raise bb.build.FuncFailed, "%s inherits useradd but doesn't set USERADD_PACKAGES" % d.getVar('FILE')
 
-    for pkg in useradd_packages.split():
-        if not d.getVar('USERADD_PARAM_%s' % pkg, True) and not d.getVar('GROUPADD_PARAM_%s' % pkg, True):
-            raise bb.build.FuncFailed, "%s inherits useradd but doesn't set USERADD_PARAM or GROUPADD_PARAM for package %s" % (d.getVar('FILE'), pkg)
+	for pkg in useradd_packages.split():
+		if not d.getVar('USERADD_PARAM_%s' % pkg, True) and not d.getVar('GROUPADD_PARAM_%s' % pkg, True):
+			raise bb.build.FuncFailed, "%s inherits useradd but doesn't set USERADD_PARAM or GROUPADD_PARAM for package %s" % (d.getVar('FILE'), pkg)
 
 python __anonymous() {
-    update_useradd_after_parse(d)
+	update_useradd_after_parse(d)
 }
 
 # Return a single [GROUP|USER]ADD_PARAM formatted string which includes the
 # [group|user]add parameters for all USERADD_PACKAGES in this recipe
 def get_all_cmd_params(d, cmd_type):
-    import string
-    
-    param_type = cmd_type.upper() + "ADD_PARAM_%s"
-    params = []
+	import string
+	
+	param_type = cmd_type.upper() + "ADD_PARAM_%s"
+	params = []
 
-    useradd_packages = d.getVar('USERADD_PACKAGES', True) or ""
-    for pkg in useradd_packages.split():
-        param = d.getVar(param_type % pkg, True)
-        if param:
-            params.append(param)
+	useradd_packages = d.getVar('USERADD_PACKAGES', True) or ""
+	for pkg in useradd_packages.split():
+		param = d.getVar(param_type % pkg, True)
+		if param:
+			params.append(param)
 
-    return string.join(params, "; ")
+	return string.join(params, "; ")
 
 # Adds the preinst script into generated packages
 fakeroot python populate_packages_prepend () {
-    def update_useradd_package(pkg):
-        bb.debug(1, 'adding user/group calls to preinst for %s' % pkg)
-
-        """
-        useradd preinst is appended here because pkg_preinst may be
-        required to execute on the target. Not doing so may cause
-        useradd preinst to be invoked twice, causing unwanted warnings.
-        """
-        preinst = d.getVar('pkg_preinst_%s' % pkg, True) or d.getVar('pkg_preinst', True)
-        if not preinst:
-            preinst = '#!/bin/sh\n'
-        preinst += d.getVar('useradd_preinst', True)
-        d.setVar('pkg_preinst_%s' % pkg, preinst)
-
-        # RDEPENDS setup
-        rdepends = d.getVar("RDEPENDS_%s" % pkg, True) or ""
-        rdepends += ' ' + d.getVar('MLPREFIX') + 'base-passwd'
-        rdepends += ' ' + d.getVar('MLPREFIX') + 'shadow'
-        d.setVar("RDEPENDS_%s" % pkg, rdepends)
-
-    # Add the user/group preinstall scripts and RDEPENDS requirements
-    # to packages specified by USERADD_PACKAGES
-    if not bb.data.inherits_class('nativesdk', d):
-        useradd_packages = d.getVar('USERADD_PACKAGES', True) or ""
-        for pkg in useradd_packages.split():
-            update_useradd_package(pkg)
+	def update_useradd_package(pkg):
+		bb.debug(1, 'adding user/group calls to preinst for %s' % pkg)
+
+		"""
+		useradd preinst is appended here because pkg_preinst may be
+		required to execute on the target. Not doing so may cause
+		useradd preinst to be invoked twice, causing unwanted warnings.
+		"""
+		preinst = d.getVar('pkg_preinst_%s' % pkg, True) or d.getVar('pkg_preinst', True)
+		if not preinst:
+			preinst = '#!/bin/sh\n'
+		preinst += d.getVar('useradd_preinst', True)
+		d.setVar('pkg_preinst_%s' % pkg, preinst)
+
+		# RDEPENDS setup
+		rdepends = d.getVar("RDEPENDS_%s" % pkg, True) or ""
+		rdepends += ' ' + d.getVar('MLPREFIX') + 'base-passwd'
+		rdepends += ' ' + d.getVar('MLPREFIX') + 'shadow'
+		d.setVar("RDEPENDS_%s" % pkg, rdepends)
+
+	# Add the user/group preinstall scripts and RDEPENDS requirements
+	# to packages specified by USERADD_PACKAGES
+	if not bb.data.inherits_class('nativesdk', d):
+		useradd_packages = d.getVar('USERADD_PACKAGES', True) or ""
+		for pkg in useradd_packages.split():
+			update_useradd_package(pkg)
 }
-- 
1.7.4

Re: [PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[Richard Purdie]

On Sun, 2012-07-22 at 14:53 +0800, jackie.huang@windriver.com wrote:
> From: Jackie Huang <jackie.huang@windriver.com>
> 
> A race condition can occur when adding users and groups to the
> passwd and group files, in [YOCTO #1794], 10 times retry added
> but it is not fixed completely.
> 
> This fix re-writes the useradd_preinst and useradd_sysroot with
> python and use locking of bb.utils to lock the passwd and group
> files before executing useradd/groupadd commands to avoid the
> lock race themselves.
> 
> [YOCTO #2779]
> 
> Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
> ---
>  meta/classes/useradd.bbclass |  284 ++++++++++++++++++------------------------
>  1 files changed, 124 insertions(+), 160 deletions(-)

Please resend this with the whitespace issues resolved. Its near
impossible to review as it stands :(

Cheers,

Richard

Re: [PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[Huang, Jie (Jackie)]

>On Sun, 2012-07-22 at 14:53 +0800, jackie.huang@windriver.com wrote:
>> From: Jackie Huang <jackie.huang@windriver.com>
>>
>> A race condition can occur when adding users and groups to the
>>passwd and group files, in [YOCTO #1794], 10 times retry added
>> but it is not fixed completely.
>>
>> This fix re-writes the useradd_preinst and useradd_sysroot with
>> python and use locking of bb.utils to lock the passwd and group
>> files before executing useradd/groupadd commands to avoid the
>> lock race themselves.
>>
>> [YOCTO #2779]
>>
>> Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
>> ---
>>  meta/classes/useradd.bbclass |  284 ++++++++++++++++++------------------------
>>  1 files changed, 124 insertions(+), 160 deletions(-)
>
>Please resend this with the whitespace issues resolved. Its near
>impossible to review as it stands :(

Re-sent, sorry about that.

Thanks,
Jackie

>
>Cheers,
>
>Richard


_______________________________________________
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core

Re: [PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[Randy MacLeod]

On 12-07-22 08:10 AM, Huang, j (Jackie) wrote:
>
>
>> On Sun, 2012-07-22 at 14:53 +0800, jackie.huang@windriver.com wrote:
>>> From: Jackie Huang <jackie.huang@windriver.com>
>>>
>>> A race condition can occur when adding users and groups to the
>>> passwd and group files, in [YOCTO #1794], 10 times retry added
>>> but it is not fixed completely.
>>>
>>> This fix re-writes the useradd_preinst and useradd_sysroot with
>>> python and use locking of bb.utils to lock the passwd and group
>>> files before executing useradd/groupadd commands to avoid the
>>> lock race themselves.
>>>
>>> [YOCTO #2779]
>>>
>>> Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
>>> ---
>>>   meta/classes/useradd.bbclass |  284 ++++++++++++++++++------------------------
>>>   1 files changed, 124 insertions(+), 160 deletions(-)
>>
>> Please resend this with the whitespace issues resolved. Its near
>> impossible to review as it stands :(
>
> Re-sent, sorry about that.

Hi Jackie,

I don't see that your new version sent at 8:01 is any better
but I could be wrong...

Robert,
Please take a look and help Jackie with formatting
and perhaps small single purpose commits as needed -
i.e. separate the whitespace changes from the functional changes.

This problem is happening frequently so I'd like to get this
worked out upstream.

Thanks,
// Randy


>
> Thanks,
> Jackie
>
>>
>> Cheers,
>>
>> Richard
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>


-- 
# Randy MacLeod. MTS, Linux, Wind River
Direct: 613.963.1350

Re: [PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[Randy MacLeod]

On 12-07-22 09:56 PM, Randy MacLeod wrote:
> On 12-07-22 08:10 AM, Huang, j (Jackie) wrote:
>>
>>
>>> On Sun, 2012-07-22 at 14:53 +0800, jackie.huang@windriver.com wrote:
>>>> From: Jackie Huang <jackie.huang@windriver.com>
>>>>
>>>> A race condition can occur when adding users and groups to the
>>>> passwd and group files, in [YOCTO #1794], 10 times retry added
>>>> but it is not fixed completely.
>>>>
>>>> This fix re-writes the useradd_preinst and useradd_sysroot with
>>>> python and use locking of bb.utils to lock the passwd and group
>>>> files before executing useradd/groupadd commands to avoid the
>>>> lock race themselves.
>>>>
>>>> [YOCTO #2779]
>>>>
>>>> Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
>>>> ---
>>>>   meta/classes/useradd.bbclass |  284
>>>> ++++++++++++++++++------------------------
>>>>   1 files changed, 124 insertions(+), 160 deletions(-)
>>>
>>> Please resend this with the whitespace issues resolved. Its near
>>> impossible to review as it stands :(
>>
>> Re-sent, sorry about that.
>
> Hi Jackie,
>
> I don't see that your new version sent at 8:01 is any better
> but I could be wrong...
>
> Robert,
> Please take a look and help Jackie with formatting
> and perhaps small single purpose commits as needed -
> i.e. separate the whitespace changes from the functional changes.

Summary:
    Jackie did the right thing; I need to read more carefully. ;-)

// Randy

Details:

I've looked at the two emails more closely now and see the
white space problem lines and saw that Jackie did fix those up
in the new patch. Specifically:

-USERADDSETSCENEDEPS = "${MLPREFIX}base-passwd:do_populate_sy
+USERADDSETSCENEDEPS = "base-passwd:do_populate_sysroot_setsc
  USERADDSETSCENEDEPS_virtclass-cross = ""
  USERADDSETSCENEDEPS_virtclass-native = ""
  USERADDSETSCENEDEPS_virtclass-nativesdk = ""
@@ -154,61 +118,61 @@ do_package_setscene[depends] = "${USERA

  # Recipe parse-time sanity checks
  def update_useradd_after_parse(d):
-    useradd_packages = d.getVar('USERADD_PACKAGES', True)
+       useradd_packages = d.getVar('USERADD_PACKAGES', True)

-    if not useradd_packages:
-        raise bb.build.FuncFailed, "%s inherits useradd but
+       if not useradd_packages:
+               raise bb.build.FuncFailed, "%s inherits usera

-    for pkg in useradd_packages.split():
-        if not d.getVar('USERADD_PARAM_%s' % pkg, True) and
-            raise bb.build.FuncFailed, "%s inherits useradd
+       for pkg in useradd_packages.split():
+               if not d.getVar('USERADD_PARAM_%s' % pkg, Tru
+                       raise bb.build.FuncFailed, "%s inheri

  python __anonymous() {
-    update_useradd_after_parse(d)
+       update_useradd_after_parse(d)
  }

Sorry for the noise.

// Randy



>
> This problem is happening frequently so I'd like to get this
> worked out upstream.
>
> Thanks,
> // Randy
>
>
>>
>> Thanks,
>> Jackie
>>
>>>
>>> Cheers,
>>>
>>> Richard
>>
>>
>> _______________________________________________
>> Openembedded-core mailing list
>> Openembedded-core@lists.openembedded.org
>> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>>
>> _______________________________________________
>> Openembedded-core mailing list
>> Openembedded-core@lists.openembedded.org
>> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>>
>
>


-- 
# Randy MacLeod. MTS, Linux, Wind River
Direct: 613.963.1350

Re: [PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[Richard Purdie]

On Sun, 2012-07-22 at 14:53 +0800, jackie.huang@windriver.com wrote:
> From: Jackie Huang <jackie.huang@windriver.com>
> 
> A race condition can occur when adding users and groups to the
> passwd and group files, in [YOCTO #1794], 10 times retry added
> but it is not fixed completely.
> 
> This fix re-writes the useradd_preinst and useradd_sysroot with
> python and use locking of bb.utils to lock the passwd and group
> files before executing useradd/groupadd commands to avoid the
> lock race themselves.
> 
> [YOCTO #2779]
> 
> Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
> ---
>  meta/classes/useradd.bbclass |  284 ++++++++++++++++++------------------------
>  1 files changed, 124 insertions(+), 160 deletions(-)
> 
> diff --git a/meta/classes/useradd.bbclass b/meta/classes/useradd.bbclass
> index bb8f42b..ed5ed69 100644
> --- a/meta/classes/useradd.bbclass
> +++ b/meta/classes/useradd.bbclass
> @@ -14,126 +14,90 @@ USERADDDEPENDS_virtclass-nativesdk = ""
>  # c) As the preinst script in the target package at do_rootfs time
>  # d) As the preinst script in the target package on device as a package upgrade
>  #
> -useradd_preinst () {
> -OPT=""
> -SYSROOT=""
> -
> -if test "x$D" != "x"; then

[...]

> -	done
> -fi
> +def useradd_preinst(d):
> +	import re
> +

[...]


> -    # Add the user/group preinstall scripts and RDEPENDS requirements
> -    # to packages specified by USERADD_PACKAGES
> -    if not bb.data.inherits_class('nativesdk', d):
> -        useradd_packages = d.getVar('USERADD_PACKAGES', True) or ""
> -        for pkg in useradd_packages.split():
> -            update_useradd_package(pkg)
> +	def update_useradd_package(pkg):
> +		bb.debug(1, 'adding user/group calls to preinst for %s' % pkg)
> +
> +		"""
> +		useradd preinst is appended here because pkg_preinst may be
> +		required to execute on the target. Not doing so may cause
> +		useradd preinst to be invoked twice, causing unwanted warnings.
> +		"""
> +		preinst = d.getVar('pkg_preinst_%s' % pkg, True) or d.getVar('pkg_preinst', True)
> +		if not preinst:
> +			preinst = '#!/bin/sh\n'
> +		preinst += d.getVar('useradd_preinst', True)


This looks like we're adding the contents of the useradd_preinst
function (changed from shell to python) to a script headed with
"#!/bin/sh"? Python script with a /bin/sh header isn't a good idea.

We can't really depend on python being on the target device to add users
in the postinstall script either.

So in summary, this patch needs a lot more thought and hasn't been well
tested.

python functions should also be 4 space indented.

Cheers,

Richard

Re: [PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[jhuang0]

On 7/23/2012 6:09 PM, Richard Purdie wrote:
> On Sun, 2012-07-22 at 14:53 +0800, jackie.huang@windriver.com wrote:
>> From: Jackie Huang<jackie.huang@windriver.com>
>>
>> A race condition can occur when adding users and groups to the
>> passwd and group files, in [YOCTO #1794], 10 times retry added
>> but it is not fixed completely.
>>
>> This fix re-writes the useradd_preinst and useradd_sysroot with
>> python and use locking of bb.utils to lock the passwd and group
>> files before executing useradd/groupadd commands to avoid the
>> lock race themselves.
>>
>> [YOCTO #2779]
>>
>> Signed-off-by: Jackie Huang<jackie.huang@windriver.com>
>> ---
>>   meta/classes/useradd.bbclass |  284 ++++++++++++++++++------------------------
>>   1 files changed, 124 insertions(+), 160 deletions(-)
>>
>> diff --git a/meta/classes/useradd.bbclass b/meta/classes/useradd.bbclass
>> index bb8f42b..ed5ed69 100644
>> --- a/meta/classes/useradd.bbclass
>> +++ b/meta/classes/useradd.bbclass
>> @@ -14,126 +14,90 @@ USERADDDEPENDS_virtclass-nativesdk = ""
>>   # c) As the preinst script in the target package at do_rootfs time
>>   # d) As the preinst script in the target package on device as a package upgrade
>>   #
>> -useradd_preinst () {
>> -OPT=""
>> -SYSROOT=""
>> -
>> -if test "x$D" != "x"; then
>
> [...]
>
>> -	done
>> -fi
>> +def useradd_preinst(d):
>> +	import re
>> +
>
> [...]
>
>
>> -    # Add the user/group preinstall scripts and RDEPENDS requirements
>> -    # to packages specified by USERADD_PACKAGES
>> -    if not bb.data.inherits_class('nativesdk', d):
>> -        useradd_packages = d.getVar('USERADD_PACKAGES', True) or ""
>> -        for pkg in useradd_packages.split():
>> -            update_useradd_package(pkg)
>> +	def update_useradd_package(pkg):
>> +		bb.debug(1, 'adding user/group calls to preinst for %s' % pkg)
>> +
>> +		"""
>> +		useradd preinst is appended here because pkg_preinst may be
>> +		required to execute on the target. Not doing so may cause
>> +		useradd preinst to be invoked twice, causing unwanted warnings.
>> +		"""
>> +		preinst = d.getVar('pkg_preinst_%s' % pkg, True) or d.getVar('pkg_preinst', True)
>> +		if not preinst:
>> +			preinst = '#!/bin/sh\n'
>> +		preinst += d.getVar('useradd_preinst', True)
>
>
> This looks like we're adding the contents of the useradd_preinst
> function (changed from shell to python) to a script headed with
> "#!/bin/sh"? Python script with a /bin/sh header isn't a good idea.

Hi Richard,

This patch you replied is the first one I sent with whitespace issues, I 
didn't change the function update_useradd_package.

>
> We can't really depend on python being on the target device to add users
> in the postinstall script either.
>
> So in summary, this patch needs a lot more thought and hasn't been well
> tested.

You're right, I didn't think about that it will be script running on the 
target device and I just tested for tow different contexts:
1) Before do_install
2) At do_populate_sysroot_setscene when installing from sstate packages

>
> python functions should also be 4 space indented.
>

I will change with 4 space indent.

Thanks,
Jackie


> Cheers,
>
> Richard
>
>
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
>

-- 
Jackie Huang
WIND RIVER | China Development Center
MSN:jackielily@hotmail.com
Tel: +86 8477 8594
Mobile: +86 138 1027 4745

[PATCH 0/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[jackie.huang]

From: Jackie Huang <jackie.huang@windriver.com>

A race condition can occur when adding users and groups to the
passwd and group files, in [YOCTO #1794], 10 times retry added
but it is not fixed completely.

This fix re-writes the useradd_preinst and useradd_sysroot with
python and use locking of bb.utils to lock the passwd and group
files before executing useradd/groupadd commands to avoid the
lock race themselves.

[YOCTO #2779]

Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
---
The following changes since commit 1eae5b6c87cd4f825dba9d2526b34231d33b1e92:

  python-native: Use append instead of += so the lsb override for EXTRA_OECONF works as expected (2012-07-22 12:07:53 +0100)

are available in the git repository at:
  git://git.pokylinux.org/poky-contrib jhuang0/yocto_2779_groupadd_0722_v2
  http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=jhuang0/yocto_2779_groupadd_0722_v2

Jackie Huang (1):
  useradd.bbclass: use locking of bb.utils to avoid lock race issue of
    useradd/groupadd

 meta/classes/useradd.bbclass |  200 +++++++++++++++++------------------------
 1 files changed, 83 insertions(+), 117 deletions(-)

-- 
1.7.4

[PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[jackie.huang]

From: Jackie Huang <jackie.huang@windriver.com>

A race condition can occur when adding users and groups to the
passwd and group files, in [YOCTO #1794], 10 times retry added
but it is not fixed completely.

This fix re-writes the useradd_preinst and useradd_sysroot with
python and use locking of bb.utils to lock the passwd and group
files before executing useradd/groupadd commands to avoid the
lock race themselves.

[YOCTO #2779]

Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
---
 meta/classes/useradd.bbclass |  200 +++++++++++++++++------------------------
 1 files changed, 83 insertions(+), 117 deletions(-)

diff --git a/meta/classes/useradd.bbclass b/meta/classes/useradd.bbclass
index bb8f42b..81bcb56 100644
--- a/meta/classes/useradd.bbclass
+++ b/meta/classes/useradd.bbclass
@@ -14,126 +14,92 @@ USERADDDEPENDS_virtclass-nativesdk = ""
 # c) As the preinst script in the target package at do_rootfs time
 # d) As the preinst script in the target package on device as a package upgrade
 #
-useradd_preinst () {
-OPT=""
-SYSROOT=""
-
-if test "x$D" != "x"; then
-	# Installing into a sysroot
-	SYSROOT="$D"
-	OPT="--root $D"
-
-	# Add groups and users defined for all recipe packages
-	GROUPADD_PARAM="${@get_all_cmd_params(d, 'group')}"
-	USERADD_PARAM="${@get_all_cmd_params(d, 'user')}"
-else
-	# Installing onto a target
-	# Add groups and users defined only for this package
-	GROUPADD_PARAM="${GROUPADD_PARAM}"
-	USERADD_PARAM="${USERADD_PARAM}"
-fi
-
-# Perform group additions first, since user additions may depend
-# on these groups existing
-if test "x$GROUPADD_PARAM" != "x"; then
-	echo "Running groupadd commands..."
-	# Invoke multiple instances of groupadd for parameter lists
-	# separated by ';'
-	opts=`echo "$GROUPADD_PARAM" | cut -d ';' -f 1`
-	remaining=`echo "$GROUPADD_PARAM" | cut -d ';' -f 2-`
-	while test "x$opts" != "x"; do
-		groupname=`echo "$opts" | awk '{ print $NF }'`
-		group_exists=`grep "^$groupname:" $SYSROOT/etc/group || true`
-		if test "x$group_exists" = "x"; then
-			count=1
-			while true; do
-				eval $PSEUDO groupadd $OPT $opts || true
-				group_exists=`grep "^$groupname:" $SYSROOT/etc/group || true`
-				if test "x$group_exists" = "x"; then
-					# File locking issues can require us to retry the command
-					echo "WARNING: groupadd command did not succeed. Retrying..."
-					sleep 1
-				else
-					break
-				fi
-				count=`expr $count + 1`
-				if test $count = 11; then
-					echo "ERROR: tried running groupadd command 10 times without success, giving up"
-					exit 1
-				fi
-			done		
-		else
-			echo "Note: group $groupname already exists, not re-creating it"
-		fi
-
-		if test "x$opts" = "x$remaining"; then
-			break
-		fi
-		opts=`echo "$remaining" | cut -d ';' -f 1`
-		remaining=`echo "$remaining" | cut -d ';' -f 2-`
-	done
-fi 
-
-if test "x$USERADD_PARAM" != "x"; then
-	echo "Running useradd commands..."
-	# Invoke multiple instances of useradd for parameter lists
-	# separated by ';'
-	opts=`echo "$USERADD_PARAM" | cut -d ';' -f 1`
-	remaining=`echo "$USERADD_PARAM" | cut -d ';' -f 2-`
-	while test "x$opts" != "x"; do
-		# useradd does not have a -f option, so we have to check if the
-		# username already exists manually
-		username=`echo "$opts" | awk '{ print $NF }'`
-		user_exists=`grep "^$username:" $SYSROOT/etc/passwd || true`
-		if test "x$user_exists" = "x"; then
-			count=1
-			while true; do
-				eval $PSEUDO useradd $OPT $opts || true
-				user_exists=`grep "^$username:" $SYSROOT/etc/passwd || true`
-				if test "x$user_exists" = "x"; then
-					# File locking issues can require us to retry the command
-					echo "WARNING: useradd command did not succeed. Retrying..."
-					sleep 1
-				else
-					break
-				fi
-				count=`expr $count + 1`
-				if test $count = 11; then
-					echo "ERROR: tried running useradd command 10 times without success, giving up"
-					exit 1
-				fi
-			done
-		else
-			echo "Note: username $username already exists, not re-creating it"
-		fi
-
-		if test "x$opts" = "x$remaining"; then
-			break
-		fi
-		opts=`echo "$remaining" | cut -d ';' -f 1`
-		remaining=`echo "$remaining" | cut -d ';' -f 2-`
-	done
-fi
+def useradd_preinst(d):
+	import re
+	import commands
+	import os
+
+	sysroot = ""
+	opt = ""
+
+	dir = d.getVar('STAGING_DIR_TARGET', True)
+	if dir:
+		# Installing into a sysroot
+		sysroot = dir
+		opt = "--root %s" % dir
+
+		# Add groups and users defined for all recipe packages
+		groupadd_param = get_all_cmd_params(d, 'group')
+		useradd_param = get_all_cmd_params(d, 'user')
+	else:
+		# Installing onto a target
+		# Add groups and users defined only for this package
+		groupadd_param = d.getVar('GROUPADD_PARAM', True)
+		useradd_param = d.getVar('GROUPADD_PARAM', True)
+
+	group_file = '%s/etc/group' % sysroot
+	user_file = '%s/etc/passwd' % sysroot
+
+	# Use the locking of bb to the group/passwd file to avoid the
+	# locking issue of groupadd/useradd
+	group_lock = '%s.locked' % group_file
+	user_lock = '%s.locked' % user_file
+	lockfiles = [group_lock, user_lock]
+
+	with bb.utils.fileslocked(lockfiles):
+		# Perform group additions first, since user additions may depend
+		# on these groups existing
+		if groupadd_param and sysroot:
+			bb.debug(1, "Running groupadd commands ...")
+			# Invoke multiple instances of groupadd for parameter lists
+			# separated by ';'
+			param_list = groupadd_param.split(';')
+			for opts in param_list:
+				groupname = opts.split()[-1]
+				with open(group_file, 'r') as f:
+					passwd_lines = f.read()
+				group_re = re.compile('\n%s' % groupname)
+				if group_re.search(passwd_lines):
+					bb.note("Note: groupname %s already exists, not re-creating it" % groupname)
+					continue
+				try:
+					output, err = bb.process.run('groupadd %s %s' % (opt, opts))
+				except bb.process.CmdError as exc:
+					bb.error("Failed to add group: %s" % exc)
+				else:
+					bb.note("Successful to add group %s" % groupname)
+
+		if useradd_param:
+			bb.debug(1, "Running useradd commands ...")
+			# Invoke multiple instances of useradd for parameter lists
+			# separated by ';'
+			param_list = useradd_param.split(';')
+			for opts in param_list:
+				# useradd does not have a -f option, so we have to check if the
+				# username already exists manually
+				username = opts.split()[-1]
+				with open(user_file, 'r') as f:
+					passwd_lines = f.read()
+				user_re = re.compile('\n%s' % username)
+				if user_re.search(passwd_lines):
+					bb.note("Note: username %s already exists, not re-creating it" % username)
+					continue
+				try:
+					output, err = bb.process.run('useradd %s %s' % (opt, opts))
+				except bb.process.CmdError as exc:
+					bb.error("Failed to add user: %s" % exc)
+				else:
+					bb.note("Successful to add user %s" % username)
+
+fakeroot python useradd_sysroot () {
+	useradd_preinst(d)
 }
 
-useradd_sysroot () {
-	# Pseudo may (do_install) or may not (do_populate_sysroot_setscene) be running 
-	# at this point so we're explicit about the environment so pseudo can load if 
-	# not already present.
-	export PSEUDO="${FAKEROOTENV} PSEUDO_LOCALSTATEDIR=${STAGING_DIR_TARGET}${localstatedir}/pseudo ${STAGING_DIR_NATIVE}${bindir}/pseudo"
-
-	# Explicitly set $D since it isn't set to anything
-	# before do_install
-	D=${STAGING_DIR_TARGET}
-	useradd_preinst
+fakeroot python useradd_sysroot_sstate () {
+    if d.getVar("BB_CURRENTTASK", True) == "package_setscene":
+        useradd_preinst(d)
 }
 
-useradd_sysroot_sstate () {
-	if [ "${BB_CURRENTTASK}" = "package_setscene" ]
-	then
-		useradd_sysroot
-	fi
-}
 
 do_install[prefuncs] += "${SYSROOTFUNC}"
 SYSROOTFUNC = "useradd_sysroot"
@@ -171,7 +137,7 @@ python __anonymous() {
 # [group|user]add parameters for all USERADD_PACKAGES in this recipe
 def get_all_cmd_params(d, cmd_type):
     import string
-    
+
     param_type = cmd_type.upper() + "ADD_PARAM_%s"
     params = []
 
-- 
1.7.4