From owner-freebsd-hackers@freebsd.org Thu Oct 8 09:47:37 2015 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 0A2039D16D6 for ; Thu, 8 Oct 2015 09:47:37 +0000 (UTC) (envelope-from scdbackup@gmx.net) Received: from mout.gmx.net (mout.gmx.net [212.227.15.18]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mout.gmx.net", Issuer "TeleSec ServerPass DE-1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 8F07ACE7 for ; Thu, 8 Oct 2015 09:47:36 +0000 (UTC) (envelope-from scdbackup@gmx.net) Received: from scdbackup.webframe.org ([91.63.124.210]) by mail.gmx.com (mrgmx001) with ESMTPSA (Nemesis) id 0Lt2BW-1alX0P31nW-012YWn for ; Thu, 08 Oct 2015 11:47:27 +0200 Date: Thu, 08 Oct 2015 11:49:01 +0200 From: "Thomas Schmitt" To: freebsd-hackers@freebsd.org Subject: What to do with triaged Coverity complaints about makefs ? Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <302195821622251047657@scdbackup.webframe.org> X-Provags-ID: V03:K0:znj2Abs5be8Do0fZT8pREw9YWeUBLOjnZ9H6IaDJrAbkIJKco9y XUs87SiJbrCGXwjTsqLKF56Dn2/d0g1SQE13hdEOFzHZ/Tj33G5VAXrL0vnaMI7JBrZJAC2 Xwc9arOgLFMylSzWcGHL7xKhoNuo9lFAdq9HUIK5KRoMuq9WUPIErB9uGKn/zZOv+c+2TDo lsVVwclyRaEH2tMNJk0lA== X-UI-Out-Filterresults: notjunk:1;V01:K0:6k17Vd3YgGQ=:+iaMcyMtzeELBd9gVGoskA Tdjj86SXjyrk4VoEiEXkJWyWs4MuIqmVSp4lfAW2HFeMXWPi89uisaqBiT9OpvGlv8kiIo+wH r+64i2hz2hbNKvJzE/7GwveNM+fqlH0blb+kpnwNXTUdckAmjmCw9VpCVEhygqLDwjvpdW4V0 PLgOsnH7gxIGSrFV6K90XxX2m2VviqiBJtXhkRQR8XslGgA1/cgQCctryMqng6ILe81Q/F/+k /WIUFHcpzBrwOHRXsIkTMUHNXrtykOfJo5ALESS67E1SEC9V5z4HX3WVbn0AUzb/f4xr9yRzZ 5AAL9+5mz+P6tKaHng7ownvOHBlvrv3ftytWwDkQFrjjIfuFht4PtP+H4hAsB48l+6knE1mx0 RyViiQ97Em/MQH13OeOtUWQf5i2rtkqCli3Pu7DdumZ2tQJNKMseCt/uRVI7oFuX90SIry9F8 1VdMAYRjO5p2H892NmVOENt6+nEqBLmUZkH3aWWRrL74/CP2HaRkW6lbat665DPGDtco63L9d 5sJCprHPIABQTZWa8CUNvYhVfHVFsRy5X0tWNw0/DpX6Vptsnk04tKMMj2IZNz8yVfYyU/32I EfQtsl36WTtsyJBZqh5jFBgd9eq9NGIfk84M6NgbrcYMIINNy/WhZ5ePs6gk1MALjPOOT0oW/ sFSYlwl8Du5E+jb3iuleIzNVUYGApUD/cj3t2spEoKSt1tsa+XsqM3MQgSuWuL8JUMb6dy/L+ Ma27EZSqF4zmptL354rvGpLpmE69t0gpODpPErq3LWakHRy0Q7ETh4p+HrhzEWWGTmkaqMISr jqK7K18 X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Oct 2015 09:47:37 -0000 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("%i\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