Fix segmentation fault in tailf on 32 bit

PATCH In review Tobias Stoeckmann 3 participants 7 messages Jul 10, 2016 – Jul 19, 2016 Util-Linux package

Share & tools

Tobias Stoeckmann

author

2016-07-10 14:14 UTC #0 lore raw

PATCH 1/1 Fix segmentation fault in tailf on 32 bit

tailf crashes with a segmentation fault when used with a file that is exactly 4GB in size due to an integer overflow between off_t and size_t:

$ dd if=/dev/zero of=tailf.crash bs=1 count=1 seek=4294967295
$ tailf tailf.crash
Segmentation fault
$ _

Signed-off-by Tobias Stoeckmann <[email protected]>

---
 text-utils/tailf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

text-utils/tailf.c +3 -2

index ea082c7..e9ba49b 100644
@@ -42,6 +42,7 @@
 #include <errno.h>
 #include <getopt.h>
 #include <sys/mman.h>
+#include <limits.h>
 
 #ifdef HAVE_INOTIFY_INIT
 #include <sys/inotify.h>

@@ -55,7 +56,7 @@
 
 #define DEFAULT_LINES  10
 
-/* st->st_size has to be greater than zero! */
+/* st->st_size has to be greater than zero and smaller or equal to SIZE_MAX! */
 static void tailf(const char *filename, size_t lines, struct stat *st)
 {
 	int fd;

@@ -281,7 +282,7 @@ int main(int argc, char **argv)
 		err(EXIT_FAILURE, _("stat of %s failed"), filename);
 	if (!S_ISREG(st.st_mode))
 		errx(EXIT_FAILURE, _("%s: is not a file"), filename);
-	if (st.st_size)
+	if (st.st_size && st.st_size <= SIZE_MAX)
 		tailf(filename, lines, &st);
 
 #ifdef HAVE_INOTIFY_INIT

Signature · 1 line

2.9.0

Karel Zak

maintainer

2016-07-14 10:12 UTC #1 lore raw

Quoted 2 lines · Tobias Stoeckmann

 text-utils/tailf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Applied with a small change, thanks.

Karel

Signature · 2 lines

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

Tobias Stoeckmann

author

2016-07-14 19:28 UTC #2 lore raw

PATCH 1/1 Fix previously adjusted segfault patch

Casting the value to be checked to size_t renders the check useless. If st_size is SIZE_MAX+1, it will be truncated to 0 and the check succeeds. In fact, this check can never be false because every value stored in a size_t is smaller or equal to SIZE_MAX.

I think this adjustment was meant to fix a compiler warning for 64 bit systems for which sizeof(off_t) is sizeof(size_t), but the signedness differs. One possibility is to do a "binary and" operation with the value SIZE_MAX. If the original value and the and-operated value differ, it means that a higher bit was set and therefore the file was too large.

Signed-off-by Tobias Stoeckmann <[email protected]>

---
 text-utils/tailf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

text-utils/tailf.c +1 -1

index 6219aa2..5ddbee9 100644
@@ -284,7 +284,7 @@ int main(int argc, char **argv)
 		errx(EXIT_FAILURE, _("%s: is not a file"), filename);
 
 	/* mmap is based on size_t */
-	if (st.st_size && (size_t) st.st_size <= SIZE_MAX)
+	if (st.st_size && st.st_size == (st.st_size & SIZE_MAX))
 		tailf(filename, lines, &st);
 
 #ifdef HAVE_INOTIFY_INIT

Signature · 1 line

2.9.0

Karel Zak

maintainer

2016-07-15 11:16 UTC #3 lore raw

Re: [PATCH] Fix previously adjusted segfault patch

Quoted 6 lines · Tobias Stoeckmann

Casting the value to be checked to size_t renders the check useless. If st_size is SIZE_MAX+1, it will be truncated to 0 and the check succeeds. In fact, this check can never be false because every value stored in a size_t is smaller or equal to SIZE_MAX.
I think this adjustment was meant to fix a compiler warning for 64 bit

yes

Quoted 15 lines

systems for which sizeof(off_t) is sizeof(size_t), but the signedness differs. One possibility is to do a "binary and" operation with the value SIZE_MAX. If the original value and the and-operated value differ, it means that a higher bit was set and therefore the file was too large.
Signed-off-by Tobias Stoeckmann <[email protected]>
---
 text-utils/tailf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
text-utils/tailf.c +0 -0
index 6219aa2..5ddbee9 100644

text-utils/tailf.c int main(int argc, char **argv) · line 287

Quoted 5 lines

text-utils/tailf.c +1 -1

@@ -284,7 +284,7 @@ int main(int argc, char **argv)
 		errx(EXIT_FAILURE, _("%s: is not a file"), filename);
 
 	/* mmap is based on size_t */
-	if (st.st_size && (size_t) st.st_size <= SIZE_MAX)
+	if (st.st_size && st.st_size == (st.st_size & SIZE_MAX))

but compiler still don't like it, because "& SIZE_MAX" result is unsigned number.

text-utils/tailf.c: In function ‘main’:
text-utils/tailf.c:287:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

I guess you need to cast:

    if (st.st_size > 0 && st.st_size == (off_t) (st.st_size & SIZE_MAX))

Maybe it would be nice to have a macros for this purpose in include/c.h, something like:

#define is_between(x, mi, ma) (x >= mi && x == (__typeof__(x)) (x & ma))

#define is_sizet_convertible(x) is_between(x, 0, SIZE_MAX)
#define is_uint32_convertible(x) is_between(x, 0, UINT32_MAX)
...

    if (st.st_size && is_sizet_convertible(st.st_size))
        tailf(filename, lines, &st);

Correct? or maybe I need a coffee? ;-)

Karel

Signature · 2 lines

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

Tobias Stoeckmann

author

2016-07-16 10:51 UTC #4 lore raw

Re: [PATCH] Fix previously adjusted segfault patch

I think this adjustment was meant to fix a compiler warning for 64 bit systems for which sizeof(off_t) is sizeof(size_t), but the signedness differs.

Going unconditionally to the greatest possible unsigned int type if st_size is positive (off_t is signed) will fix this issue.

Signed-off-by Tobias Stoeckmann <[email protected]>

---
This should be an easy fix without adding too much boiler-plate code. Your macros also imply that every value would be allowed as upper limit, but in fact it would have to be a 2^x-1 value, otherwise the bit pattern check would turn out to be invalid.

---
 text-utils/tailf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

text-utils/tailf.c +1 -1

index 6219aa2..5937f73 100644
@@ -284,7 +284,7 @@ int main(int argc, char **argv)
 		errx(EXIT_FAILURE, _("%s: is not a file"), filename);
 
 	/* mmap is based on size_t */
-	if (st.st_size && (size_t) st.st_size <= SIZE_MAX)
+	if (st.st_size > 0 && (uintmax_t) st.st_size <= (uintmax_t) SIZE_MAX)
 		tailf(filename, lines, &st);
 
 #ifdef HAVE_INOTIFY_INIT

Signature · 1 line

2.9.1

Karel Zak

maintainer Applied

2016-07-19 09:05 UTC #5 lore raw

Re: [PATCH] Fix previously adjusted segfault patch

Quoted 2 lines · Tobias Stoeckmann

 text-utils/tailf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks!

Karel

Signature · 2 lines

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

Ruediger Meier

2016-07-19 09:49 UTC #6 lore raw

In reply to Tobias Stoeckmann

Quoted 3 lines · Tobias Stoeckmann

tailf crashes with a segmentation fault when used with a file that is exactly 4GB in size due to an integer overflow between off_t and size_t:

Note that tailf is deprecated and will be removed in a few months. It's better to use tail -f (from coreutils),

cu,
Rudi