[U-Boot] [PATCH] binman: ensure temp filenames don't collide
From: Stephen Warren swarren@nvidia.com
The U-Boot Makefile can invoke binman multiple times in parallel. This is problematic because binman uses a static hard-coded temporary file name. If two instances of binman use that filename at the same time, one writing one reading, they may silently read the wrong content or actively detect missing signatures and error out the build process. Fix this by using a PID-specific filename instead.
Signed-off-by: Stephen Warren swarren@nvidia.com --- tools/binman/control.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/binman/control.py b/tools/binman/control.py index a40b300fdacb..515999278949 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -121,7 +121,7 @@ def Binman(options, args): # output into a file in our output directly. Then scan it for use # in binman. dtb_fname = fdt_util.EnsureCompiled(dtb_fname) - fname = tools.GetOutputFilename('u-boot-out.dtb') + fname = tools.GetOutputFilename('u-boot-out.dtb') + str(os.getpid()) with open(dtb_fname) as infd: with open(fname, 'wb') as outfd: outfd.write(infd.read())
Hi Stephen,
On 16 July 2018 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
The U-Boot Makefile can invoke binman multiple times in parallel. This is problematic because binman uses a static hard-coded temporary file name. If two instances of binman use that filename at the same time, one writing one reading, they may silently read the wrong content or actively detect missing signatures and error out the build process. Fix this by using a PID-specific filename instead.
Signed-off-by: Stephen Warren swarren@nvidia.com
tools/binman/control.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/binman/control.py b/tools/binman/control.py index a40b300fdacb..515999278949 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -121,7 +121,7 @@ def Binman(options, args): # output into a file in our output directly. Then scan it for use # in binman. dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
fname = tools.GetOutputFilename('u-boot-out.dtb')
fname = tools.GetOutputFilename('u-boot-out.dtb') + str(os.getpid()) with open(dtb_fname) as infd: with open(fname, 'wb') as outfd: outfd.write(infd.read())-- 2.18.0
But the output directory is itself (normally) a temporary dir. That determines the directly which GetOutputFilename() uses. So I am not sure how this can happen in practice?
Regards, Simon
On 07/18/2018 07:32 PM, Simon Glass wrote:
Hi Stephen,
On 16 July 2018 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
The U-Boot Makefile can invoke binman multiple times in parallel. This is problematic because binman uses a static hard-coded temporary file name. If two instances of binman use that filename at the same time, one writing one reading, they may silently read the wrong content or actively detect missing signatures and error out the build process. Fix this by using a PID-specific filename instead.
Signed-off-by: Stephen Warren swarren@nvidia.com
tools/binman/control.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/binman/control.py b/tools/binman/control.py index a40b300fdacb..515999278949 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -121,7 +121,7 @@ def Binman(options, args): # output into a file in our output directly. Then scan it for use # in binman. dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
fname = tools.GetOutputFilename('u-boot-out.dtb')
fname = tools.GetOutputFilename('u-boot-out.dtb') + str(os.getpid()) with open(dtb_fname) as infd: with open(fname, 'wb') as outfd: outfd.write(infd.read())-- 2.18.0
But the output directory is itself (normally) a temporary dir. That determines the directly which GetOutputFilename() uses. So I am not sure how this can happen in practice?
IIRC, the output directory for all 3 files was the same; the root of the build output directory. Perhaps the fact I run "make O=build-xxx" rather than just "make" makes a difference?
Hi Stephen,
On 19 July 2018 at 15:14, Stephen Warren swarren@wwwdotorg.org wrote:
On 07/18/2018 07:32 PM, Simon Glass wrote:
Hi Stephen,
On 16 July 2018 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
The U-Boot Makefile can invoke binman multiple times in parallel. This is problematic because binman uses a static hard-coded temporary file name. If two instances of binman use that filename at the same time, one writing one reading, they may silently read the wrong content or actively detect missing signatures and error out the build process. Fix this by using a PID-specific filename instead.
Signed-off-by: Stephen Warren swarren@nvidia.com
tools/binman/control.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/binman/control.py b/tools/binman/control.py index a40b300fdacb..515999278949 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -121,7 +121,7 @@ def Binman(options, args): # output into a file in our output directly. Then scan it for use # in binman. dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
fname = tools.GetOutputFilename('u-boot-out.dtb')
fname = tools.GetOutputFilename('u-boot-out.dtb') + str(os.getpid()) with open(dtb_fname) as infd: with open(fname, 'wb') as outfd: outfd.write(infd.read())-- 2.18.0
But the output directory is itself (normally) a temporary dir. That determines the directly which GetOutputFilename() uses. So I am not sure how this can happen in practice?
IIRC, the output directory for all 3 files was the same; the root of the build output directory. Perhaps the fact I run "make O=build-xxx" rather than just "make" makes a difference?
Yes you are right - the -O flag sets the output directory and it no-longer uses a temporary directory.
But if we add a PID to every filename then we end up with multiple output files and we don't know which is right.
I think the correct fix is to change the Makefile to only run binman once. It makes no sense to run it multiple times.
Regards, Simon
On 07/19/2018 08:17 PM, Simon Glass wrote:
Hi Stephen,
On 19 July 2018 at 15:14, Stephen Warren swarren@wwwdotorg.org wrote:
On 07/18/2018 07:32 PM, Simon Glass wrote:
Hi Stephen,
On 16 July 2018 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
The U-Boot Makefile can invoke binman multiple times in parallel. This is problematic because binman uses a static hard-coded temporary file name. If two instances of binman use that filename at the same time, one writing one reading, they may silently read the wrong content or actively detect missing signatures and error out the build process. Fix this by using a PID-specific filename instead.
Signed-off-by: Stephen Warren swarren@nvidia.com
tools/binman/control.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/binman/control.py b/tools/binman/control.py index a40b300fdacb..515999278949 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -121,7 +121,7 @@ def Binman(options, args): # output into a file in our output directly. Then scan it for use # in binman. dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
fname = tools.GetOutputFilename('u-boot-out.dtb')
fname = tools.GetOutputFilename('u-boot-out.dtb') + str(os.getpid()) with open(dtb_fname) as infd: with open(fname, 'wb') as outfd: outfd.write(infd.read())-- 2.18.0
But the output directory is itself (normally) a temporary dir. That determines the directly which GetOutputFilename() uses. So I am not sure how this can happen in practice?
IIRC, the output directory for all 3 files was the same; the root of the build output directory. Perhaps the fact I run "make O=build-xxx" rather than just "make" makes a difference?
Yes you are right - the -O flag sets the output directory and it no-longer uses a temporary directory.
But if we add a PID to every filename then we end up with multiple output files and we don't know which is right.
I think the correct fix is to change the Makefile to only run binman once. It makes no sense to run it multiple times.
IIRC, this filename is just a temp file that eventually gets removed after binman runs. Certainly there are not *.${pid} files left around after the build has completed.
Hi Stephen,
On 19 July 2018 at 22:43, Stephen Warren swarren@wwwdotorg.org wrote:
On 07/19/2018 08:17 PM, Simon Glass wrote:
Hi Stephen,
On 19 July 2018 at 15:14, Stephen Warren swarren@wwwdotorg.org wrote:
On 07/18/2018 07:32 PM, Simon Glass wrote:
Hi Stephen,
On 16 July 2018 at 16:51, Stephen Warren swarren@wwwdotorg.org
wrote:
From: Stephen Warren swarren@nvidia.com
The U-Boot Makefile can invoke binman multiple times in parallel. This is problematic because binman uses a static hard-coded temporary file name. If two instances of binman use that filename at the same time,
one
writing one reading, they may silently read the wrong content or
actively
detect missing signatures and error out the build process. Fix this by using a PID-specific filename instead.
Signed-off-by: Stephen Warren swarren@nvidia.com
tools/binman/control.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/binman/control.py b/tools/binman/control.py index a40b300fdacb..515999278949 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -121,7 +121,7 @@ def Binman(options, args): # output into a file in our output directly. Then scan
it for use
# in binman. dtb_fname = fdt_util.EnsureCompiled(dtb_fname)
fname = tools.GetOutputFilename('u-boot-out.dtb')
fname = tools.GetOutputFilename('u-boot-out.dtb') +str(os.getpid())
with open(dtb_fname) as infd: with open(fname, 'wb') as outfd: outfd.write(infd.read())-- 2.18.0
But the output directory is itself (normally) a temporary dir. That determines the directly which GetOutputFilename() uses. So I am not sure how this can happen in practice?
IIRC, the output directory for all 3 files was the same; the root of the build output directory. Perhaps the fact I run "make O=build-xxx" rather than just "make" makes a difference?
Yes you are right - the -O flag sets the output directory and it no-longer uses a temporary directory.
But if we add a PID to every filename then we end up with multiple output files and we don't know which is right.
I think the correct fix is to change the Makefile to only run binman once. It makes no sense to run it multiple times.
IIRC, this filename is just a temp file that eventually gets removed after binman runs. Certainly there are not *.${pid} files left around after the build has completed.
This is what I see with your patch:
u-boot-out.dtb198273 u-boot-out.dtb198277 u-boot-out.dtb198280
I think there are two options:
1. Add an arg to binman to indicate which output image to create, so that binman runs three times but only one of those generates the .dtb output file.
2. Use a GNU make pattern rule, as described here: https://www.gnu.org/software/make/manual/html_node/Pattern-Examples.html#Pat...
I'll send a patch for the latter which seems easier to me. But I will probably have to update binman to support (1) too.
Regards, Simon
participants (2)
-
Simon Glass -
Stephen Warren