GNU bug report logs - #49209
coreutils: stack out-of-bounds write in tail --follow

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 49209 in the body.
You can then email your comments to 49209 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: bug-coreutils <at> gnu.org
Cc: sbroz <at> redhat.com
Subject: coreutils: stack out-of-bounds write in tail --follow
Date: Thu, 24 Jun 2021 16:26:33 +0200

Hello,

As originally reported by Stepan Broz (CC'd), tail --follow crashes when it
is given too many files to follow, and ulimit -n is set to >1024.

FD_SET(wd, &rfd) in tail_forever_inotify() writes beyond the stack-allocated 
variable in case wd >= FD_SETSIZE.  Minimal example:

# mkdir dir
# cd dir
# touch {1..1021}
# ulimit -n 1025
# tail -f *

The out-of-bound write could be fixed like this:

--- a/src/tail.c
+++ b/src/tail.c
@@ -1647,28 +1647,32 @@ tail_forever_inotify (int wd, struct File_spec *f, 
size_t n_files,
               if (writer_is_dead)
                 exit (EXIT_SUCCESS);

               writer_is_dead = (kill (pid, 0) != 0 && errno != EPERM);

               if (writer_is_dead)
                 delay.tv_sec = delay.tv_usec = 0;
               else
                 {
                   delay.tv_sec = (time_t) sleep_interval;
                   delay.tv_usec = 1000000 * (sleep_interval - delay.tv_sec);
                 }
             }

+           if (FD_SETSIZE <= wd)
+             die (EXIT_FAILURE, 0,
+                  _("too many open files to wait for inotify events"));
+
            fd_set rfd;
            FD_ZERO (&rfd);
            FD_SET (wd, &rfd);
            if (monitor_output)
              FD_SET (STDOUT_FILENO, &rfd);

            int file_change = select (MAX (wd, STDOUT_FILENO) + 1,
                                      &rfd, NULL, NULL, pid ? &delay: NULL);

            if (file_change == 0)
              continue;
            else if (file_change == -1)
              die (EXIT_FAILURE, errno,
                   _("error waiting for inotify and output events"));


Alternatively, we might rewrite the code to use poll() rather than select().

Kamil

Message #8 received at 49209 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Kamil Dudka <kdudka <at> redhat.com>, 49209 <at> debbugs.gnu.org
Cc: sbroz <at> redhat.com
Subject: Re: bug#49209: coreutils: stack out-of-bounds write in tail --follow
Date: Thu, 24 Jun 2021 15:50:25 +0100

On 24/06/2021 15:26, Kamil Dudka wrote:
> Hello,
> 
> As originally reported by Stepan Broz (CC'd), tail --follow crashes when it
> is given too many files to follow, and ulimit -n is set to >1024.
> 
> FD_SET(wd, &rfd) in tail_forever_inotify() writes beyond the stack-allocated
> variable in case wd >= FD_SETSIZE.  Minimal example:
> 
> # mkdir dir
> # cd dir
> # touch {1..1021}
> # ulimit -n 1025
> # tail -f *
> 
> The out-of-bound write could be fixed like this:
> 
> --- a/src/tail.c
> +++ b/src/tail.c
> @@ -1647,28 +1647,32 @@ tail_forever_inotify (int wd, struct File_spec *f,
> size_t n_files,
>                 if (writer_is_dead)
>                   exit (EXIT_SUCCESS);
> 
>                 writer_is_dead = (kill (pid, 0) != 0 && errno != EPERM);
> 
>                 if (writer_is_dead)
>                   delay.tv_sec = delay.tv_usec = 0;
>                 else
>                   {
>                     delay.tv_sec = (time_t) sleep_interval;
>                     delay.tv_usec = 1000000 * (sleep_interval - delay.tv_sec);
>                   }
>               }
> 
> +           if (FD_SETSIZE <= wd)
> +             die (EXIT_FAILURE, 0,
> +                  _("too many open files to wait for inotify events"));
> +
>              fd_set rfd;
>              FD_ZERO (&rfd);
>              FD_SET (wd, &rfd);
>              if (monitor_output)
>                FD_SET (STDOUT_FILENO, &rfd);
> 
>              int file_change = select (MAX (wd, STDOUT_FILENO) + 1,
>                                        &rfd, NULL, NULL, pid ? &delay: NULL);
> 
>              if (file_change == 0)
>                continue;
>              else if (file_change == -1)
>                die (EXIT_FAILURE, errno,
>                     _("error waiting for inotify and output events"));
> 
> 
> Alternatively, we might rewrite the code to use poll() rather than select().

Note the number of descriptors select() is waiting on in independent of the number of files.
We should be able to inotify_init() earlier in the process to avoid this issue.
I'll have a look.

thanks!
Pádraig

Message #11 received at 49209 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: sbroz <at> redhat.com, 49209 <at> debbugs.gnu.org
Subject: Re: bug#49209: coreutils: stack out-of-bounds write in tail --follow
Date: Thu, 24 Jun 2021 17:07:20 +0200

On Thursday, June 24, 2021 4:50:25 PM CEST Pádraig Brady wrote:
> Note the number of descriptors select() is waiting on in independent of the
> number of files. We should be able to inotify_init() earlier in the process
> to avoid this issue. I'll have a look.

Good idea!  This could make it work instead of throwing an error.  
Nevertheless, the boundary check should be added anyway as long as we use 
FD_SET(), because the number of first available file descriptor can still
be controlled from outside.

Kamil

Message #14 received at 49209 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>,
 Kamil Dudka <kdudka <at> redhat.com>, 49209 <at> debbugs.gnu.org
Cc: sbroz <at> redhat.com
Subject: Re: bug#49209: coreutils: stack out-of-bounds write in tail --follow
Date: Thu, 24 Jun 2021 08:50:16 -0700

On 6/24/21 7:50 AM, Pádraig Brady wrote:
> We should be able to inotify_init() earlier in the process to avoid this 
> issue.

inotify_init can return 1025 even if called first thing, so we also need 
to dup2 the result of early inotify_init down to 3 (or whatever), or at 
least to check that it's less than 1024. Choosing 3 is a tricky 
business, since it's not clear what fds the C library actually needs.

Message #19 received at 49209-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Stepan Broz <sbroz <at> redhat.com>
Cc: Kamil Dudka <kdudka <at> redhat.com>,
 Pádraig Brady <P <at> draigBrady.com>, 49209-done <at> debbugs.gnu.org
Subject: Re: bug#49209: coreutils: stack out-of-bounds write in tail --follow
Date: Sat, 26 Jun 2021 18:47:46 -0700

[Message part 1 (text/plain, inline)]

On 6/24/21 8:50 AM, Paul Eggert wrote:

> inotify_init can return 1025 even if called first thing, so we also need 
> to dup2 the result of early inotify_init down to 3 (or whatever), or at 
> least to check that it's less than 1024. Choosing 3 is a tricky 
> business, since it's not clear what fds the C library actually needs.

When looking into this I decided it was cleaner to fix coreutils by 
using 'poll' instead of 'select', as Kamil suggested. I installed the 
attached patches to do that. The last patch fixes the bug.

Thanks for reporting the problem.

[0001-maint-while-1-while-true.patch (text/x-patch, attachment)]

[0002-tail-fix-abuse2-test-race.patch (text/x-patch, attachment)]

[0003-tail-use-poll-not-select.patch (text/x-patch, attachment)]

Message #22 received at 49209-done <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Stepan Broz <sbroz <at> redhat.com>,
 Pádraig Brady <P <at> draigbrady.com>,
 49209-done <at> debbugs.gnu.org
Subject: Re: bug#49209: coreutils: stack out-of-bounds write in tail --follow
Date: Mon, 28 Jun 2021 17:10:00 +0200

On Sunday, June 27, 2021 3:47:46 AM CEST Paul Eggert wrote:
> When looking into this I decided it was cleaner to fix coreutils by
> using 'poll' instead of 'select', as Kamil suggested. I installed the
> attached patches to do that. The last patch fixes the bug.

This works for me.  Thank you for taking care of the fix!

Kamil

Message #25 received at 49209 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 49209 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu, kdudka <at> redhat.com
Subject: Re: bug#49209: coreutils: stack out-of-bounds write in tail --follow
Date: Tue, 29 Jun 2021 00:10:42 +0100

On 27/06/2021 02:47, Paul Eggert wrote:
> On 6/24/21 8:50 AM, Paul Eggert wrote:
> 
>> inotify_init can return 1025 even if called first thing, so we also need
>> to dup2 the result of early inotify_init down to 3 (or whatever), or at
>> least to check that it's less than 1024. Choosing 3 is a tricky
>> business, since it's not clear what fds the C library actually needs.
> 
> When looking into this I decided it was cleaner to fix coreutils by
> using 'poll' instead of 'select', as Kamil suggested. I installed the
> attached patches to do that. The last patch fixes the bug.

Yes using poll() with the inotify descriptor is cleaner.
That's limited to Linux also, so should work fine.

For my reference, with the change from select() to poll() in check_output_alive(),
we'll need to be more carefully test tests/tail-2/pipe-f.sh on various platforms,
especially those where we implement missing poll (mingw, MSVC 14, HP NonStop).
If poll() didn't work here for these platforms (and we moved back to using select),
we might considering removing poll as a dependency as it would be redundant.

thanks for the fix!
Pádraig

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.