Fix #243: Heap overflow in logic2_display_loop()

Make sure that all tests from check_file_actlst() function are done on a
data file, even if this file has activities with unknown formats.
"sadf -H" and, e.g., "sadf -Hx" should as a consequence behave the same.

Signed-off-by: Sebastien GODARD <sysstat@users.noreply.github.com>

diff --git a/format.c b/format.c
index 77717aabd5fd54344c4c39b654e95ed6bdfcf7ff..84521b90cfce176e04e9d4dc800e73bd993cbf13 100644 (file)
--- a/format.c
+++ b/format.c
@@ -40,7 +40,7 @@
  */
 struct report_format hdr_fmt = {
        .id             = F_HEADER_OUTPUT,
-       .options        = FO_HEADER_ONLY + FO_BAD_FILE_FORMAT,
+       .options        = FO_HEADER_ONLY,
        .f_header       = print_hdr_header,
        .f_statistics   = NULL,
        .f_timestamp    = NULL,
@@ -112,7 +112,7 @@ struct report_format json_fmt = {
  */
 struct report_format conv_fmt = {
        .id             = F_CONV_OUTPUT,
-       .options        = FO_BAD_FILE_FORMAT,
+       .options        = 0,
        .f_header       = NULL,
        .f_statistics   = NULL,
        .f_timestamp    = NULL,

diff --git a/sa.h b/sa.h
index 3c3a0c7e10334bc8e83f364c5517833f9d7146a6..615b1f4843efbeb9339dae19665a0d7ba8ed6a53 100644 (file)
--- a/sa.h
+++ b/sa.h
@@ -1446,7 +1446,7 @@ int check_disk_reg
        (struct activity *, int, int, int);
 void check_file_actlst
        (int *, char *, struct activity * [], uint64_t, struct file_magic *,
-        struct file_header *, struct file_activity **, unsigned int [], int, int *, int *);
+        struct file_header *, struct file_activity **, unsigned int [], int *, int *);
 int check_net_dev_reg
        (struct activity *, int, int, int);
 int check_net_edev_reg

diff --git a/sa_common.c b/sa_common.c
index 856a371550d28de2304322b76a34eb13417eae78..87e1e0f6882295ddfaebd0368b211f25720a7de8 100644 (file)
--- a/sa_common.c
+++ b/sa_common.c
@@ -1922,9 +1922,6 @@ int sa_open_read_magic(int *fd, char *dfile, struct file_magic *file_magic,
  * @dfile      Name of system activity data file.
  * @act                Array of activities.
  * @flags      Flags for common options and system state.
- * @ignore     Set to 1 if a true sysstat activity file but with a bad
- *             format should not yield an error message. Used with
- *             sadf -H (sadf -c doesn't call check_file_actlst() function).
  *
  * OUT:
  * @ifd                System activity data file descriptor.
@@ -1942,16 +1939,17 @@ int sa_open_read_magic(int *fd, char *dfile, struct file_magic *file_magic,
 void check_file_actlst(int *ifd, char *dfile, struct activity *act[], uint64_t flags,
                       struct file_magic *file_magic, struct file_header *file_hdr,
                       struct file_activity **file_actlst, unsigned int id_seq[],
-                      int ignore, int *endian_mismatch, int *arch_64)
+                      int *endian_mismatch, int *arch_64)
 {
-       int i, j, k, p;
+       int i, j, k, p, skip;
        struct file_activity *fal;
        void *buffer = NULL;
        size_t bh_size = FILE_HEADER_SIZE;
        size_t ba_size = FILE_ACTIVITY_SIZE;
 
        /* Open sa data file and read its magic structure */
-       if (sa_open_read_magic(ifd, dfile, file_magic, ignore, endian_mismatch, TRUE) < 0)
+       if (sa_open_read_magic(ifd, dfile, file_magic,
+                              DISPLAY_HDR_ONLY(flags), endian_mismatch, TRUE) < 0)
                /*
                 * Not current sysstat's format.
                 * Return now so that sadf -H can display at least
@@ -2077,17 +2075,19 @@ void check_file_actlst(int *ifd, char *dfile, struct activity *act[], uint64_t f
                        /* Unknown activity */
                        continue;
 
-               if (act[p]->magic != fal->magic) {
+               skip = FALSE;
+               if (fal->magic != act[p]->magic) {
                        /* Bad magical number */
-                       if (ignore) {
+                       if (DISPLAY_HDR_ONLY(flags)) {
                                /*
                                 * This is how sadf -H knows that this
                                 * activity has an unknown format.
                                 */
                                act[p]->magic = ACTIVITY_MAGIC_UNKNOWN;
                        }
-                       else
-                               continue;
+                       else {
+                               skip = TRUE;
+                       }
                }
 
                /* Check max value for known activities */
@@ -2112,7 +2112,16 @@ void check_file_actlst(int *ifd, char *dfile, struct activity *act[], uint64_t f
                     ||
                     ((fal->types_nr[0] <= act[p]->gtypes_nr[0]) &&
                     (fal->types_nr[1] <= act[p]->gtypes_nr[1]) &&
-                    (fal->types_nr[2] <= act[p]->gtypes_nr[2]))) && !ignore) {
+                    (fal->types_nr[2] <= act[p]->gtypes_nr[2]))) &&
+                    (act[p]->magic != ACTIVITY_MAGIC_UNKNOWN) && !DISPLAY_HDR_ONLY(flags)) {
+                       /*
+                        * This may not be an error (that's actually why we may have changed
+                        * the magic number for this activity above).
+                        * So, if the activity magic number has changed (e.g.: ACTIVITY_MAGIC_UNKNOWN)
+                        * and we want to display only the header, then ignore the error.
+                        * If we want to also display the stats then we must stop here because
+                        * we won't know how to map the contents of the stats structure.
+                        */
 #ifdef DEBUG
                        fprintf(stderr, "%s: id=%d file=%d,%d,%d activity=%d,%d,%d\n",
                                __FUNCTION__, fal->id, fal->types_nr[0], fal->types_nr[1], fal->types_nr[2],
@@ -2129,6 +2138,13 @@ void check_file_actlst(int *ifd, char *dfile, struct activity *act[], uint64_t f
                        goto format_error;
                }
 
+               if (skip)
+                       /*
+                        * This is an unknown activity and we want stats about it:
+                        * This is not possible so skip it.
+                        */
+                       continue;
+
                for (k = 0; k < 3; k++) {
                        act[p]->ftypes_nr[k] = fal->types_nr[k];
                }

diff --git a/sadf.c b/sadf.c
index a931b09b3903c07ba9f5246f96d0af3017c6ac81..34e317ba24fdf2bdbfea9d3e41a6f1d2ae4cae7f 100644 (file)
--- a/sadf.c
+++ b/sadf.c
@@ -1446,12 +1446,11 @@ void read_stats_from_file(char dfile[], char pcparchive[])
        struct file_magic file_magic;
        struct file_activity *file_actlst = NULL;
        struct tm rectime;
-       int ifd, ignore, tab = 0;
+       int ifd, tab = 0;
 
        /* Prepare file for reading and read its headers */
-       ignore = ACCEPT_BAD_FILE_FORMAT(fmt[f_position]->options);
        check_file_actlst(&ifd, dfile, act, flags, &file_magic, &file_hdr,
-                         &file_actlst, id_seq, ignore, &endian_mismatch, &arch_64);
+                         &file_actlst, id_seq, &endian_mismatch, &arch_64);
 
        if (DISPLAY_HDR_ONLY(flags)) {
                if (*fmt[f_position]->f_header) {

diff --git a/sadf.h b/sadf.h
index c845c4d2920252b104767eb6f711303809f49cfc..60eaa9f0706bceee3630fdbe73ac7b6459ec803d 100644 (file)
--- a/sadf.h
+++ b/sadf.h
@@ -54,11 +54,7 @@
  */
 #define FO_HEADER_ONLY         0x02
 
-/*
- * Indicate that a true sysstat activity file but with a bad
- * format should not yield an error message.
- */
-#define FO_BAD_FILE_FORMAT     0x04
+/* Unused: 0x04 */
 
 /*
  * Indicate that timestamp can be displayed in local time instead of UTC
@@ -110,7 +106,6 @@
 
 #define SET_LC_NUMERIC_C(m)            (((m) & FO_LC_NUMERIC_C)        == FO_LC_NUMERIC_C)
 #define ACCEPT_HEADER_ONLY(m)          (((m) & FO_HEADER_ONLY)         == FO_HEADER_ONLY)
-#define ACCEPT_BAD_FILE_FORMAT(m)      (((m) & FO_BAD_FILE_FORMAT)     == FO_BAD_FILE_FORMAT)
 #define ACCEPT_LOCAL_TIME(m)           (((m) & FO_LOCAL_TIME)          == FO_LOCAL_TIME)
 #define ACCEPT_HORIZONTALLY(m)         (((m) & FO_HORIZONTALLY)        == FO_HORIZONTALLY)
 #define ACCEPT_SEC_EPOCH(m)            (((m) & FO_SEC_EPOCH)           == FO_SEC_EPOCH)

diff --git a/sar.c b/sar.c
index d946a497ee8c8bf6bd16661a74303247cb4af8b2..9eeeeb2c237e8fca8193edce422d90d65eb1f400 100644 (file)
--- a/sar.c
+++ b/sar.c
@@ -1001,7 +1001,7 @@ void read_stats_from_file(char from_file[])
 
        /* Read file headers and activity list */
        check_file_actlst(&ifd, from_file, act, flags, &file_magic, &file_hdr,
-                         &file_actlst, id_seq, FALSE, &endian_mismatch, &arch_64);
+                         &file_actlst, id_seq, &endian_mismatch, &arch_64);
 
        /* Perform required allocations */
        allocate_structures(act);