Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 08 Oct 2015 11:49:01 +0200
From:      "Thomas Schmitt" <scdbackup@gmx.net>
To:        freebsd-hackers@freebsd.org
Subject:   What to do with triaged Coverity complaints about makefs ?
Message-ID:  <302195821622251047657@scdbackup.webframe.org>

next in thread | raw e-mail | index | archive | help
Hi,

i now get to see the makefs complaints of Coverity.
(The trick was to try logging out, which failed, but then produced
 the list of complaints and made the GUI willing to show details.)

Many complaints are about the FFS aspect of makefs, of which i
have no clue.
Some more are about /usr.sbin/makefs/cd9660/cd9660_debug.c,
of which i do not really understand the purpose.
So i mainly cared for bugs related to ISO 9660 production.

Two ISO 9660 related complaints are easy to understand, but
i currently lack the makefs background to decide how to
react on the miserable failure to write to disk.

Meanwhile i got 9 diagnoses with remedy proposals (if real bug)
and 2 diagnoses without proposals.


Now what to do with my findings ?

File 6 PRs for 974635+974636, 976312, 977470, 978431, 1008927,
1305659 ?

Fill out the Coverity "Triage" window at the right side of the
GUI ? (Am i entitled to do more than reading there ?)


==============================================================
Overview:

974635 : DIRTY HACK : Copying several struct elements by single memcpy().

974636 : DIRTY HACK : Copying several struct elements by single memcpy().

975347 : NO DISK FULL PROVISIONS : How to react on failed fseek() ?

975348 : NO DISK FULL PROVISIONS : How to react on failed fseek() ?

975621 : FALSE POSITIVE :

976312 : SIGSEGV : Processing is unaware of exotic option.

976924 : FALSE POSITIVE :

977470 : SPECS VIOLATION : Writing slightly wrong Boot Record.

978431 : MEMORY LEAK : No free() after malloc().

1008927 : WRONG SIZEOF CHECK : Comparing bit count rather than byte count.

1305659 : MAYBE SIGSEGV : Unclear whether reaction on malloc failure suffices.

==============================================================
Information sources:

Coverity report:
  https://scan6.coverity.com/reports.htm#v36539/p10022
  with File filter "*makefs*"

Source obtained by:
  svn co  https://svn.FreeBSD.org/base/head/usr.sbin/makefs makefs

Wider FreeBSD code studied via:
  https://svnweb.freebsd.org/base/head/

==============================================================
974635 : DIRTY HACK : Copying several struct elements by single memcpy().
--------------------------------------------------------------

usr.sbin/makefs/ffs/ffs_bswap.c

CID 974635 : Destination buffer too small (BUFFER_SIZE)
  10. buffer_size: You might overrun the 48 byte destination
  string n->di_db by writing the maximum 60 bytes from o->di_db.

138        memcpy(n->di_db, o->di_db, (NDADDR + NIADDR) * sizeof(u_int32_t));

--------------- Source analysis:

This does not apply to ISO 9660.
But indeed sys/ufs/ufs/dinode.h defines

  #define NDADDR 12 /* Direct addresses in inode. */
  #define NIADDR 3 /* Indirect addresses in inode. */

and

  typedef int32_t ufs1_daddr_t;
  ...
  struct ufs1_dinode {
          ...
          ufs1_daddr_t di_db[NDADDR]; /* 40: Direct disk blocks. */
          ufs1_daddr_t di_ib[NIADDR]; /* 88: Indirect disk blocks. */
          ...

So both arrays get copied in one memcpy() operation.

--------------- Remedy proposal:

One should consider to use two separate memcpy() calls.
(I cannot judge whether the alignment of 40 and 88 needs
 padding bytes on any architecture.)

==============================================================
974636 : DIRTY HACK : Copying several struct elements by single memcpy().
--------------------------------------------------------------

usr.sbin/makefs/ffs/ffs_bswap.c

CID 974636 : Destination buffer too small (BUFFER_SIZE)
  20. buffer_size: You might overrun the 16 byte destination string
  n->di_extb by writing the maximum 136 bytes from o->di_extb.

168        memcpy(n->di_extb, o->di_extb, (NXADDR + NDADDR + NIADDR) * 8);

--------------- Source analysis:

Like CID 974635.

sys/ufs/ufs/dinode.h defines

  typedef int64_t ufs2_daddr_t;
  ...
  struct ufs2_dinode {
          ...
          ufs2_daddr_t di_extb[NXADDR];/* 96: External attributes block. */
          ufs2_daddr_t di_db[NDADDR]; /* 112: Direct disk blocks. */
          ufs2_daddr_t di_ib[NIADDR]; /* 208: Indirect disk blocks. */

--------------- Remedy proposal:

One should consider to use three separate memcpy() calls.

==============================================================
975347 : NO DISK FULL PROVISIONS : How to react on failed fseek() ?
--------------------------------------------------------------

usr.sbin/makefs/cd9660/cd9660_eltorito.c

CID 975347 : Unchecked return value from library (CHECKED_RETURN)
   5. check_return: Calling fseek(fd, 32UL - strlen(part_type) - 1UL, 1)
   without checking return value.

(Ouch, an ISO producer which does not work on sequential file
 objects. That's quite inconvenient for users.)

>>> How to react on failure to address ?

==============================================================
975348 : NO DISK FULL PROVISIONS : How to react on failed fseek() ?
--------------------------------------------------------------

usr.sbin/makefs/cd9660/cd9660_eltorito.c

CID 975348: Unchecked return value from library (CHECKED_RETURN)
   33. check_return: Calling fseek(fd, 510L, 0) without checking return

>>> How to react on failure to address ?

==============================================================
975621 : FALSE POSITIVE : 
--------------------------------------------------------------

usr.sbin/makefs/cd9660/cd9660_write.c

CID 975621 (#1 of 1): Copy-paste error (COPY_PASTE_ERROR)
   copy_paste_error: rf in fclose(rf) looks like a copy-paste error.
   Should it say fd instead?

471                        (void)fclose(rf);

--------------- Source analysis:

The suspicion stems from the assymetric reaction on error.
Only the read FILE pointer rf was opened inside the function.
So it gets closed in both error cases: with read FILE *rf and
with write FILE *fd.

          if (ferror(rf)) {
                  ...
                  (void)fclose(rf);
          ...
          if (ferror(fd)) {
                  ...
                  (void)fclose(rf);

==============================================================
976312 : SIGSEGV : Processing is unaware of exotic option.
--------------------------------------------------------------

usr.sbin/makefs/cd9660.c

CID 976312: Explicit null dereferenced (FORWARD_NULL)i
   4. var_deref_op: Dereferencing null pointer conversion_function.

1740        return (*conversion_function)(oldname, newname, is_file);

--------------- Source analysis:

The function pointer is non-NULL only if diskStructure.isoLevel is
1 or 2. A snippet in cd9660_parse_opts() indicates that the value
can indeed be 3:

         option_t cd9660_options[] = {
                 { "l", &diskStructure.isoLevel, 1, 3, "ISO Level" },
                 { "isolevel", &diskStructure.isoLevel, 1, 3, "ISO Level" },
                 ...

One should test makefs with option -l 3.

The line 1740 is in function cd9660_convert_filename().
ISO 9660 level 3 allows the same file names as level 2.

The use of ISO level 3 is not announced anywhere in the ISO
image but rather becomes visible only if a file is large enough
to need more than one extent. Extents can have 4 GiB - 2 KiB
of size.

The offer of ISO level 3 is questionable because neither FreeBSD
nor NetBSD can read files with multiple extents from ISO 9660.
  http://lists.freebsd.org/pipermail/freebsd-hackers/2012-April/038552.html

Surely nobody has ever created a level 3 ISO with makefs.
I doubt that it is prepared for multiple extents at all.

--------------- Remedy proposal:

Use the same conversion function for all levels above 1:

-         else if (diskStructure.isoLevel == 2)
+         else
                  conversion_function = &cd9660_level2_convert_filename;

Consider to restrict isoLevel to 1 and 2.

-                { "l", &diskStructure.isoLevel, 1, 3, "ISO Level" },
+                { "l", &diskStructure.isoLevel, 1, 2, "ISO Level" },

==============================================================
976924 : FALSE POSITIVE :
--------------------------------------------------------------

usr.sbin/makefs/cd9660.c

CID 976924: Memset fill value of '0' (NO_EFFECT)bad_memset: Memset with
   fill value '0' (the zero character).

684        memset(diskStructure.primaryDescriptor.expiration_date, 48, 16UL).
685        diskStructure.primaryDescriptor.expiration_date[16] = 0;

--------------- Source analysis:

This is correct. ECMA-11 8.4.26.1 says:
"If all characters in RBP 1 to 16 of this field are the digit ZERO
 and the number in RBP 17 is zero, it shall mean that the date and
 time are not specified."

"ZERO" means ASCII 48 = '0', "zero" means ASCII 0 = NUL.

==============================================================
977469 : 
--------------------------------------------------------------

usr.sbin/makefs/cd9660/cd9660_debug.c

CID 977469: Out-of-bounds access (OVERRUN)
   1. overrun-buffer-val: Overrunning array pttemp->parent_number
   of 2 bytes by passing it to a function which accesses it at
   byte offset 3.

186        printf("<parent_number>%i</parent_number>\n",
187            debug_get_encoded_number(pttemp->parent_number,mode));

--------------- Source analysis:

The problem is in debug_get_encoded_number() which depending
on mode reads more or less bytes.

>>>

==============================================================
977470 : SPECS VIOLATION : Writing slightly wrong Boot Record.
--------------------------------------------------------------

usr.sbin/makefs/cd9660/cd9660_eltorito.c

CID 977470: Out-of-bounds access (OVERRUN)
   2. overrun-buffer-val: Overrunning array
   diskStructure.boot_descriptor->boot_catalog_pointer of 4 bytes
   by passing it to a function which accesses it at byte offset 4.

374        cd9660_bothendian_dword(first_sector,
375                diskStructure.boot_descriptor->boot_catalog_pointer);

--------------- Source analysis:

cd9660_bothendian_dword() indeed writes 8 bytes (both endian)
into 

usr.sbin/makefs/cd9660.h defines

  typedef struct _iso9660_disk {
          ...
          boot_volume_descriptor *boot_descriptor;
          ...
  } iso9660_disk;

usr.sbin/makefs/cd9660/cd9660_eltorito.h defines

  typedef struct _boot_volume_descriptor {
          ...
          u_char boot_catalog_pointer [ISODCL(0x47,0x4A)];
          u_char unused2 [ISODCL(0x4B,0x7FF)];
  } boot_volume_descriptor;

So the overrun hits the first 4 bytes of .unused2 .

The little endian 4-byte value gets written to .boot_catalog_pointer,
even on big endian architectures. This could be very bad if used
for more computations.
But obviously this will only be written as byte string to the ISO
image.

El Torito 1.0 (1995) Figure 7 specifies bytes 0x4B to 0x7FFF
of the record as "Unused, must be 0."
But FreeBSD-11.0-CURRENT-amd64-20151001-r288459-disc1.iso
has at byte address (17 * 2048 + 0x4B) the values {0, 0, 0, 19}
which is the big endian address of the boot catalog.

--------------- Remedy proposal:

Use function cd9660_731() instead of cd9660_bothendian_dword():

-        cd9660_bothendian_dword(first_sector,
-        cd9660_731(first_sector,
                 diskStructure.boot_descriptor->boot_catalog_pointer);


==============================================================
978431 : MEMORY LEAK : No free() after malloc().
--------------------------------------------------------------

usr.sbin/makefs/cd9660/cd9660_write.c

CID 978431: Resource leak (RESOURCE_LEAK)
   9. leaked_storage: Variable buffer going out of scope leaks the
   storage it points to.

216        return cd9660_write_filedata(fd, sector, buffer_head,
217            path_table_sectors);

--------------- Source analysis:

There are two return statements in the function.
The first one (return 0) is ok, because malloc() returned NULL.
The second one uses the allocated buffer and does not free it.

--------------- Remedy proposal:

Untangle the return statement.

+        int ret;

         ...

-        return cd9660_write_filedata(fd, sector, buffer_head,
+        ret = cd9660_write_filedata(fd, sector, buffer_head,
              path_table_sectors);
+        free(buffer);
+        return ret;

==============================================================
1008927 : WRONG SIZEOF CHECK : Comparing bit count rather than byte count.
--------------------------------------------------------------

usr.sbin/makefs/cd9660/cd9660_rrip.c

CID 1008927: Operands don't affect result (CONSTANT_EXPRESSION_RESULT)
   result_independent_of_operands: (uint64_t)fnode->inode->st.st_dev >> 32
   is 0 regardless of the values of its operands. This occurs as an
   argument to a function call.

--------------- Source analysis:

The complained statement is in an if case which obviously shall
take care for 64-bit dev_t.
But the test expression looks for 256-bit dev_t (which i doubt
that it does exist anywhere).

--------------- Remedy proposal:

-        if (sizeof (fnode->inode->st.st_dev) > 32)
+        if (sizeof (fnode->inode->st.st_dev) > 4)

==============================================================
1305659 : MAYBE SIGSEGV : Unclear whether reaction on malloc failure suffices.
--------------------------------------------------------------

usr.sbin/makefs/cd9660.c

CID 1305659: Dereference before null check (REVERSE_INULL)i
  check_after_deref: Null-checking var suggests that it may be null,
  but it has already been dereferenced on all paths leading to the check.

431        if (var)
432                free(var);

--------------- Source analysis:

Indeed the function should bail out when allocation fails.

327        if ((var = strdup(option)) == NULL)
328                err(1, "allocating memory for copy of option string");

If err() does not finally call exit(), then the program runs
into a SIGSEGV by

331        val = strchr(var, '=');

The function cd9660_parse_opts() gets called by usr.sbin/makefs/makefs.c

                                if (! fstype->parse_options(p, &fsoptions))
                                        usage();

usage() calls exit(1).
So i assume that return NULL would be the way to indicate error
and cause abort. But usage() will indicate a user error where
a resource shortage is the reason.
 
(Not that i deam "!" a valid test for NULL. Why define NULL if one
 assumes it to be 0 anyway ?)

--------------- Remedy proposal:

Call exit(1) if no memory is available. (Unless you can find the
definition of err() and verify that it calls exit().)

-        if ((var = strdup(option)) == NULL)
+        if ((var = strdup(option)) == NULL) {
                 err(1, "allocating memory for copy of option string");
+                exit(1);
+        }

In any case remove the test which made Coverity suspicious.

-        if (var)
-                free(var);
+        free(var);

==============================================================


Have a nice day :)

Thomas




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?302195821622251047657>