[pacman-dev] tolerate broken logpipe

Message ID 20190710015745.32627-1-yardenack@gmail.com
State Superseded, archived
Headers show
Series
  • [pacman-dev] tolerate broken logpipe
Related show

Commit Message

Yardena Cohen July 10, 2019, 1:57 a.m. UTC
Sometimes makechrootpkg fails with:

   rm: cannot remove '/logdest/logpipe.xxxxxxxx': No such file or directory

This shouldn't cause the whole script to fail, so let's tolerate a missing pipe

Signed-off-by: Yardena Cohen <yardenack@gmail.com>
---
 scripts/makepkg.sh.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eli Schwartz July 10, 2019, 2:52 a.m. UTC | #1
On 7/9/19 9:57 PM, Yardena Cohen wrote:
> Sometimes makechrootpkg fails with:
> 
>    rm: cannot remove '/logdest/logpipe.xxxxxxxx': No such file or directory
> 
> This shouldn't cause the whole script to fail, so let's tolerate a missing pipe

As a matter of curiosity, when does this happen? I don't recall offhand
seeing it.

I'm not saying it's fundamentally wrong to make this change, but I
wouldn't have expected this to be a problem.

> Signed-off-by: Yardena Cohen <yardenack@gmail.com>
> ---
>  scripts/makepkg.sh.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index aa03e9d9..9f8c9096 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -344,7 +344,7 @@ remove_deps() {
>  
>  error_function() {
>  	if [[ -p $logpipe ]]; then
> -		rm "$logpipe"
> +		rm -f "$logpipe"

If we just checked using [[ -p namedpipe ]] then it seems like the
window for a TOCTOU is pretty narrow...

Aside for which, if we've reached this point then we're already erroring
out, so -f would only silence a warning, not make anything succeed. So
this should not be what is causing the whole script to fail.

>  	fi
>  	# first exit all subshells, then print the error
>  	if (( ! BASH_SUBSHELL )); then
> @@ -428,7 +428,7 @@ run_function() {
>  		$pkgfunc &>"$logpipe"
>  
>  		wait $teepid
> -		rm "$logpipe"
> +		rm -f "$logpipe"

I'm curious under what situation we could tee to a file, wait until tee
exits, and then try but fail to delete the file.

IIRC the only times we use run_function() are inside run_function_safe()
which sets up errexit, so if this fails then that would explain why the
script would abort, but I don't get how the file is supposed to
disappear in the first place.

>  	else
>  		"$pkgfunc"
>  	fi
>
Yardena Cohen July 10, 2019, 3:26 a.m. UTC | #2
On Tue, Jul 9, 2019 at 7:52 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
> As a matter of curiosity, when does this happen? I don't recall offhand
> seeing it.

I find it every so often, most recently building yuzu-git. I don't
know why and don't see any pattern. Googling the error message tells
me I'm not alone:

https://bbs.archlinux.org/viewtopic.php?pid=1789264#p1789264
https://github.com/AladW/aurutils/issues/19#issuecomment-194565520

I usually just solve it by patching $chroot/usr/bin/makepkg when it
happens, but I'm getting tired of doing that. :)

> If we just checked using [[ -p namedpipe ]] then it seems like the
> window for a TOCTOU is pretty narrow...
>
> Aside for which, if we've reached this point then we're already erroring
> out, so -f would only silence a warning, not make anything succeed. So
> this should not be what is causing the whole script to fail.

Good point. I was overzealous replacing both instances. Do you want a
new patch with only the second part?
Allan McRae Aug. 5, 2019, 10:47 a.m. UTC | #3
On 10/7/19 12:52 pm, Eli Schwartz wrote:
> On 7/9/19 9:57 PM, Yardena Cohen wrote:
>> Sometimes makechrootpkg fails with:
>>
>>    rm: cannot remove '/logdest/logpipe.xxxxxxxx': No such file or directory
>>
>> This shouldn't cause the whole script to fail, so let's tolerate a missing pipe
> 
> As a matter of curiosity, when does this happen? I don't recall offhand
> seeing it.
> 
> I'm not saying it's fundamentally wrong to make this change, but I
> wouldn't have expected this to be a problem.
> 
>> Signed-off-by: Yardena Cohen <yardenack@gmail.com>
>> ---
>>  scripts/makepkg.sh.in | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>> index aa03e9d9..9f8c9096 100644
>> --- a/scripts/makepkg.sh.in
>> +++ b/scripts/makepkg.sh.in
>> @@ -344,7 +344,7 @@ remove_deps() {
>>  
>>  error_function() {
>>  	if [[ -p $logpipe ]]; then
>> -		rm "$logpipe"
>> +		rm -f "$logpipe"
> 
> If we just checked using [[ -p namedpipe ]] then it seems like the
> window for a TOCTOU is pretty narrow...
> 
> Aside for which, if we've reached this point then we're already erroring
> out, so -f would only silence a warning, not make anything succeed. So
> this should not be what is causing the whole script to fail.
> 
>>  	fi
>>  	# first exit all subshells, then print the error
>>  	if (( ! BASH_SUBSHELL )); then
>> @@ -428,7 +428,7 @@ run_function() {
>>  		$pkgfunc &>"$logpipe"
>>  
>>  		wait $teepid
>> -		rm "$logpipe"
>> +		rm -f "$logpipe"
> 
> I'm curious under what situation we could tee to a file, wait until tee
> exits, and then try but fail to delete the file.
> 
> IIRC the only times we use run_function() are inside run_function_safe()
> which sets up errexit, so if this fails then that would explain why the
> script would abort, but I don't get how the file is supposed to
> disappear in the first place.
> 

I don't understand this either...  Note that there is no evidence of
this occurring when running makepkg directly.  Only under the Arch
devtools package.

I get the feeling this is papering over an issue in devtools, and am
reluctant to pull the patch unless there information to say otherwise.

Allan
Ralph Corderoy Aug. 5, 2019, 11:26 a.m. UTC | #4
Hi Allan,

> > > @@ -428,7 +428,7 @@ run_function() {
> > >            $pkgfunc &>"$logpipe"
> > >  
> > >            wait $teepid
> > > -          rm "$logpipe"
> > > +          rm -f "$logpipe"
>
> I don't understand this either...  Note that there is no evidence of
> this occurring when running makepkg directly.  Only under the Arch
> devtools package.

Strictly speaking, shouldn't that be ‘wait -f’?  I don't know if it's
only job control that could cause a change in tee's status  Also, wait's
exit status is ignored; reporting an unusual status may give a clue.
An rm without -f could be attempted first and if that fails then report
its exit status and try again with -f, checking that second rm too.

Same end result, ‘rm -f’, but perhaps more light shed on the problem.

Patch

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index aa03e9d9..9f8c9096 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -344,7 +344,7 @@  remove_deps() {
 
 error_function() {
 	if [[ -p $logpipe ]]; then
-		rm "$logpipe"
+		rm -f "$logpipe"
 	fi
 	# first exit all subshells, then print the error
 	if (( ! BASH_SUBSHELL )); then
@@ -428,7 +428,7 @@  run_function() {
 		$pkgfunc &>"$logpipe"
 
 		wait $teepid
-		rm "$logpipe"
+		rm -f "$logpipe"
 	else
 		"$pkgfunc"
 	fi