bugfindutils - Bugs: bug #64442, xargs closes stdin on receiving...

 
 

bug #64442: xargs closes stdin on receiving SIGUSR1 or SIGUSR2

Submitter:  None
Submitted:  Tue 18 Jul 2023 02:52:32 PM UTC
   
 
Category:  xargs Severity:  3 - Normal
Item Group:  Wrong result Status:  Fixed
Privacy:  Public Assigned to:  jay
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  Closed Release:  4.9.0
Fixed Release:  4.10.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 19 May 2024 12:15:05 PM UTC, comment #13: 

Fixed in git.  Patch attached.

(file #56067)

James Youngman <jay>
Group administrator
Sun 19 May 2024 10:16:35 AM UTC, comment #12: 

I attach  patch which deals with the POSIX compliance aspect explained by Geoff.

(file #56065)

James Youngman <jay>
Group administrator
Wed 19 Jul 2023 10:20:03 PM UTC, comment #11: 

It is probably more reasonable to test with USR2 as pkill will signal any xargs running on the system.

Anonymous
Wed 19 Jul 2023 10:18:12 PM UTC, comment #10: 

An automated test for this issue can be done with

sleep 0.2 && pkill -USR1 xargs & { sleep 0.5 && echo success; } | xargs


Anonymous
Wed 19 Jul 2023 03:49:23 PM UTC, comment #9: 

I decided to strace xargs on my phone and curiously read() is interrupted there as well but does not cause any problems.

(file #54945)

Anonymous
Wed 19 Jul 2023 01:57:18 PM UTC, comment #8: 

I retract my previous statement the patch works, I just accidentaly ran system xargs instead of the locally built one.

Anonymous
Wed 19 Jul 2023 01:54:35 PM UTC, comment #7: 

After testing further I found that SA_RESTART only fixes the case of waiting on read.
I managed to trigger xargs: error closing file by running this

c=1; while true; do printf '%s\n' "$c"; c=$((c+1)); done | xargs

and sending USR1 to xargs.

Anonymous
Wed 19 Jul 2023 12:36:15 PM UTC, comment #6: 

Setting sa_flags to SA_RESTART fixes this.

(file #54944)

Anonymous
Wed 19 Jul 2023 12:21:54 PM UTC, comment #5: 


From sigaction(3p)
...
The sa_flags field can be used to modify the behavior of the specified signal.
...
SA_RESTART
This flag affects the behavior of interruptible functions; that is, those specified to fail with errno set to [EINTR]. If set, and a function specified as interruptible is interrupted by this signal, the function shall restart and shall not fail with [EINTR] unless otherwise specified. If an interruptible function which uses a timeout is restarted, the duration of the timeout following the restart is set to an unspecified value that does  not exceed the original timeout value. If the flag is not set, interruptible functions interrupted by this signal shall fail with errno set to [EINTR].
...

xargs sets sa_flags to 0

xargs.c:735
xargs.c:741
  sigact.sa_flags = 0;

so read() is interrupted.

safe_read() is supposed to handle that but somehow doesn't.
Also safe_read() is supposed to but does not set SAFE_READ_ERROR for some reason.

xargs.c:1377
      /* We use safe_read here in order to avoid an error if
         SIGUSR[12] is handled during the read system call. */


Anonymous
Wed 19 Jul 2023 11:43:14 AM UTC, comment #4: 

Here is a diff between a normal xargs execution and one interrupted with SIGUSR1.
Normal execution is just running xargs and entering ^D.

--- strace-normal.txt        2023-07-19 14:31:18.639157032 +0300
+++ strace-usr1.txt        2023-07-19 14:21:40.390915386 +0300
 newfstatat(0, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(0x88, 0x9), ...}, AT_EMPTY_PATH) = 0
-read(0, "", 1024)                       = 0
+read(0, 0x565271e47710, 1024)           = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
+--- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_USER, si_pid=946742, si_uid=1000} ---
+rt_sigreturn({mask=[]})                 = -1 EINTR (Interrupted system call)
 pipe2([3, 4], 0)                        = 0
 fcntl(4, F_SETFD, FD_CLOEXEC)           = 0
-clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f9e4a386a10) = 947550
+clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7fdbe000fa10) = 946743
 close(4)                                = 0
 read(3, "", 4)                          = 0
 close(3)                                = 0
---- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=947550, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
-getpid()                                = 947549
-wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 947550
+getpid()                                = 946735
+wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 946743
+--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=946743, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
 lseek(0, 0, SEEK_CUR)                   = -1 ESPIPE (Illegal seek)
 close(0)                                = 0
+openat(AT_FDCWD, "/usr/share/locale/locale.alias", O_RDONLY|O_CLOEXEC) = 0
+newfstatat(0, "", {st_mode=S_IFREG|0644, st_size=2998, ...}, AT_EMPTY_PATH) = 0
+read(0, "# Locale name alias data base.\n#"..., 4096) = 2998
+read(0, "", 4096)                       = 0
+close(0)                                = 0
+openat(AT_FDCWD, "/usr/share/locale/en_US.UTF-8/LC_MESSAGES/findutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
+openat(AT_FDCWD, "/usr/share/locale/en_US.utf8/LC_MESSAGES/findutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
+openat(AT_FDCWD, "/usr/share/locale/en_US/LC_MESSAGES/findutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
+openat(AT_FDCWD, "/usr/share/locale/en.UTF-8/LC_MESSAGES/findutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
+openat(AT_FDCWD, "/usr/share/locale/en.utf8/LC_MESSAGES/findutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
+openat(AT_FDCWD, "/usr/share/locale/en/LC_MESSAGES/findutils.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
+write(2, "xargs: ", 7)                  = 7
+write(2, "error closing file", 18)      = 18
+write(2, "\n", 1)                       = 1
 close(1)                                = 0
 close(2)                                = 0
-exit_group(0)                           = ?
-+++ exited with 0 +++
+exit_group(1)                           = ?
++++ exited with 1 +++

*I removed lines that are the same but with different adresses due to ASLR

(file #54941, file #54942)

Anonymous
Wed 19 Jul 2023 10:59:36 AM UTC, comment #3: 

I had a quick look at the code, and the signal handler for SIGUSR1/SIGUSR2 is always setup without regard to whether or not the -P option was used.

I couldn't see why those signals would terminate xargs (but it did when I tested it on macos and linux).


raf <raf>
Wed 19 Jul 2023 10:27:02 AM UTC, comment #2: 

When fixing this, please also consider changing xargs to make it conform to POSIX as regards signal handling. POSIX does not allow xargs to handle SIGUSR1 and SIGUSR2 differently from other signals. I.e. if they are inherited as ignored they must be ignored, otherwise they must terminate xargs.

Two possible solutions:

1. Only enable special handling of SIGUSR1 and SIGUSR2 if the -P option is used (as it's not in POSIX and can therefore change the behaviour).

2. Don't enable special handling of SIGUSR1 and SIGUSR2 if POSIXLY_CORRECT is set in the environment (and -P is not used).

Geoff Clare <geoffclare>
Tue 18 Jul 2023 06:19:02 PM UTC, comment #1: 

I got same behaviour after building from findutils-4.9.0.tar.xz and master branch commit 81bcf2b9b39a107a5417867edeb7d65c731a1720

Anonymous
Tue 18 Jul 2023 02:52:32 PM UTC, original submission:  

1. the version of findutils you are using:

$ xargs --version
xargs (GNU findutils) 4.9.0
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Eric B. Decker, James Youngman, and Kevin Dalley.


2. what you were trying to do:
Move selected image files to a different location.

3. the exact command line that you used:

find . -type f | sort | sxiv -iof | tr '\n' '\0' | xargs -0rP0 mv -t ../dest


4. what you expected to happen:
On receiving SIGUSR1 or SIGUSR2 xargs adjusts its proc_max and keeps waiting for input.

5. precisely what did happen:
xargs errors with:

xargs: error closing file

exits with 1 and breaks my pipeline as selected items will not be processed anymore.

Notes:
N1. This can be reproduces by just running

$ xargs

and sending SIGUSR1 or SIGUSR2 from a different terminal

$ pkill -USR1 xargs


N2. I've pinpointed that the error message comes from gnulib/closein.c

N3. This behaviour is reproducible on my Arch Linux x86_64 machine
but isn't on Termux on my armv7l Android phone.

N4. Current workaround is to replace xargs with a while loop but it's less efficient as I have to run mv on every single item which is just less efficient.

There is also this workaround

tr '\n' '\0' | find -files0-from - -maxdepth 0 -exec mv -t ../dest +

which works for filenames but not for arbitrary input.

N5. This bug is not very critical as in most cases when I pkill xargs it has already read in all input so no error happens.
I run some scripts that process large files in parallel so sometimes I adjust parallelism with pkill -USR1(or -USR2) xargs but one time it 'killed' my xargs that was waiting on hundreds of items and I had to select them anew.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #56066:  0002-doc-Mention-the-changes-to-xargs-P.patch added by jay (3KiB - text/x-patch - documentation update for the -P patch)
file #54945:  strace-android.txt added by None (33KiB - text/plain - Strace of the termux version of xargs on Android.)
file #54944:  sa_flags.diff added by None (735B - text/x-patch)
file #54941:  strace-normal.txt added by None (5KiB - text/plain)
file #54942:  strace-usr1.txt added by None (6KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by berny (Updated the item)
  • -email is unavailable- added by jay (Updated the item)
  • -email is unavailable- added by raf (Posted a comment)
  • -email is unavailable- added by geoffclare (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-05-27 berny Open/ClosedOpen Closed
        Fixed ReleaseNone 4.10.0
    2024-05-19 jay Attached File- Added 0001-xargs-restart-stdin-reads-interruipted-by-SIGUSR1-SI.patch, #56067
        StatusNone Fixed
    2024-05-19 jay Attached File- Added 0002-doc-Mention-the-changes-to-xargs-P.patch, #56066
    2024-05-19 jay Attached File- Added 0001-xargs-POSIX-requires-SIGUSR1-2-to-be-fatal-if-not-bl.patch, #56065
    2024-05-18 jay Assigned toNone jay
    2023-07-19 None Attached File- Added strace-android.txt, #54945
    2023-07-19 None Attached File- Added sa_flags.diff, #54944
    2023-07-19 None Attached File- Added strace-normal.txt, #54941
        Attached File- Added strace-usr1.txt, #54942

    Back to the top

    Powered by Savane 3.13-a854.
    Corresponding source code