bug#49209: coreutils: stack out-of-bounds write in tail --follow
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
bug#49209: coreutils: stack out-of-bounds write in tail --follow
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
bug#49209: coreutils: stack out-of-bounds write in tail --follow
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.
From 145707949f9479f00dce41f479549f7629d7d0c9 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 26 Jun 2021 18:23:52 -0700
Subject: [PATCH 1/3] =?UTF-8?q?maint:=20while=20(1)=20=E2=86=92=20while=20?=
=?UTF-8?q?(true)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
src/basenc.c | 2 +-
src/chcon.c | 2 +-
src/chmod.c | 2 +-
src/chown-core.c | 2 +-
src/csplit.c | 2 +-
src/cut.c| 2 +-
src/date.c | 2 +-
src/dd.c | 2 +-
src/dircolors.c | 2 +-
src/du.c | 2 +-
src/expr.c | 12 ++--
src/head.c | 4 ++--
src/join.c | 2 +-
src/ls.c | 4 ++--
src/nproc.c | 2 +-
src/od.c | 6 +++---
src/ptx.c| 2 +-
src/pwd.c| 6 +++---
src/realpath.c | 2 +-
src/remove.c | 2 +-
src/rmdir.c | 2 +-
src/runcon.c | 2 +-
src/sort.c | 2 +-
src/sum.c| 2 +-
src/system.h | 2 +-
src/tac-pipe.c | 6 +++---
src/tac.c| 2 +-
src/tail.c | 12 ++--
src/tsort.c | 2 +-
29 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/src/basenc.c b/src/basenc.c
index 7e22363bf..5c97a3652 100644
--- a/src/basenc.c
+++ b/src/basenc.c
@@ -605,7 +605,7 @@ z85_encode (char const *restrict in, size_t inlen,
unsigned int val;
size_t outidx = 0;
- while (1)
+ while (true)
{
if (inlen == 0)
{
diff --git a/src/chcon.c b/src/chcon.c
index dc2258de0..88010848e 100644
--- a/src/chcon.c
+++ b/src/chcon.c
@@ -315,7 +315,7 @@ process_files (char **files, int bit_flags)
FTS *fts = xfts_open (files, bit_flags, NULL);
- while (1)
+ while (true)
{
FTSENT *ent;
diff --git a/src/chmod.c b/src/chmod.c
index 78d9c9cba..160a0c537 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -335,7 +335,7 @@ process_files (char **files, int bit_flags)
FTS *fts = xfts_open (files, bit_flags, NULL);
- while (1)
+ while (true)
{
FTSENT *ent;
diff --git a/src/chown-core.c b/src/chown-core.c
index a0b2f670f..4d816de4a 100644
--- a/src/chown-core.c
+++ b/src/chown-core.c
@@ -524,7 +524,7 @@ chown_files (char **files, int bit_flags,
FTS *fts = xfts_open (files, bit_flags | stat_flags, NULL);
- while (1)
+ while (true)
{
FTSENT *ent;
diff --git a/src/csplit.c b/src/csplit.c
index f188e8894..e1fb66ed2 100644
--- a/src/csplit.c
+++ b/src/csplit.c
@@ -496,7 +496,7 @@ load_buffer (void)
if (bytes_wanted < hold_count)
bytes_wanted = hold_count;
- while (1)
+ while (true)
{
b = get_new_buffer (bytes_wanted);
bytes_avail = b->bytes_alloc; /* Size of buffer returned. */
diff --git a/src/cut.c b/src/cut.c
index 78b82c80e..f4d44c211 100644
--- a/src/cut.c
+++ b/src/cut.c
@@ -306,7 +306,7 @@ cut_fields (FILE *stream)
That is because a non-delimited line has exactly one field. */
buffer_first_field = (suppress_non_delimited ^ !print_kth (1));
- while (1)
+ while (true)
{
if (field_idx == 1 && buffer_first_field)
{
diff --git a/src/date.c b/src/date.c
index d5eebaf25..4a7a4e243 100644
--- a/src/date.c
+++ b/src/date.c
@@ -313,7 +313,7 @@ batch_convert (char const *input_filename, char const *format,
line = NULL;
buflen = 0;
ok = true;
- while (1)
+ while (true)
{
ssize_t line_length = getline (, , in_stream);
if (line_length < 0)
diff --git a/src/dd.c b/src/dd.c
index d284357a4..fc5108f8b 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -2179,7 +2179,7 @@ dd_copy (void)
alloc_ibuf ();
alloc_obuf ();
- while (1)
+ while (true)
{
if (status_level == STATUS_PROGRESS)
{
diff --git a/src/dircolors.c b/src/dircolors.c
index fea0cdf01..b765ded9f 100644
--- a/src/dircolors.c
+++ b/src/dircolors.c
@@ -253,7 +253,7 @@ dc_parse_stream (FILE *fp, char const *filename)
if (term == NULL || *term == '\0')
term = "none";
- while (1)
+ while (true)
{
char *keywd, *arg;
bool unrecognized;
diff --git a/src/du.c b/src/du.c
index d4760a36c..efd519706 100644
--- a/src/du.c
+++ b/src/du.c
@@ -684,7 +684,7 @@ du_files (char **files, int bit_flags)
{
FTS *fts = xfts_open (files, bit_flags, NULL);
- while (1)
+ while (true)
{
FTSENT *ent;
diff --git a/src/expr.c b/src/expr.c
index ec76f7607..41185a8f8 100644
--- a/src/expr.c
+++
bug#49209: coreutils: stack out-of-bounds write in tail --follow
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.
bug#49209: coreutils: stack out-of-bounds write in tail --follow
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
bug#49209: coreutils: stack out-of-bounds write in tail --follow
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, ) 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 = 100 * (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 ();
FD_SET (wd, );
if (monitor_output)
FD_SET (STDOUT_FILENO, );
int file_change = select (MAX (wd, STDOUT_FILENO) + 1,
, NULL, NULL, pid ? : 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
bug#49209: coreutils: stack out-of-bounds write in tail --follow
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, ) 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 = 100 * (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 ();
FD_SET (wd, );
if (monitor_output)
FD_SET (STDOUT_FILENO, );
int file_change = select (MAX (wd, STDOUT_FILENO) + 1,
, NULL, NULL, pid ? : 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
