Hi all,
let me introduce myself - I'm a package maintainer of libcdio and some
other packages in Fedora and have prepared two attached patches.
The first one (libcdio-0.82-sprintf.patch) includes several fixes of
insecure sprintf calls. At least one call has led to buffer overflow
error (see https://bugzilla.redhat.com/show_bug.cgi?id=705673 for more
info and a reproducer).
The second patch (libcdio-0.82-staticanal.patch) was prepared on the
basis of a static analysis by Coverity tool. I've then checked the
mistakes and proposed fixes for them. There are mostly resource leaks,
return value checking, missing breaks etc.
If you won't found any issue concerning the fixes, please, apply the
patches to make libcdio a bit better.
Thanks a lot, cheers!
Honza
diff -up libcdio-0.82/lib/driver/MSWindows/win32_ioctl.c.sprintf
libcdio-0.82/lib/driver/MSWindows/win32_ioctl.c
--- libcdio-0.82/lib/driver/MSWindows/win32_ioctl.c.sprintf 2011-05-19
08:45:55.326672827 +0200
+++ libcdio-0.82/lib/driver/MSWindows/win32_ioctl.c 2011-05-19
08:46:00.008672826 +0200
@@ -35,7 +35,7 @@ static const char _rcsid[] = "$Id: win32
#include "undocumented.h"
#define FORMAT_ERROR(i_err, psz_msg) \
psz_msg=(char *)LocalAlloc(LMEM_ZEROINIT, 255); \
- sprintf(psz_msg, "error file %s: line %d (%s) %d\n",
+ sprintf(psz_msg, "error file %.100s: line %d (%.100s) %d\n",
_FILE__, __LINE__, __PRETTY_FUNCTION__, i_err)
#else
#include <ddk/ntddcdrm.h>
diff -up libcdio-0.82/lib/driver/solaris.c.sprintf
libcdio-0.82/lib/driver/solaris.c
--- libcdio-0.82/lib/driver/solaris.c.sprintf 2011-05-19 08:47:40.862672799
+0200
+++ libcdio-0.82/lib/driver/solaris.c 2011-05-19 09:05:47.204672524 +0200
@@ -1014,7 +1014,7 @@ cdio_get_devices_solaris (void)
/* Check if this is a directory, if so it's probably Solaris media */
if(S_ISDIR(st.st_mode)) {
- sprintf(volpath, "%s/s0", globbuf.gl_pathv[i]);
+ snprintf(volpath, 256, "%s/s0", globbuf.gl_pathv[i]);
if(stat(volpath, &st) == 0)
cdio_add_device_list(&drives, volpath, &i_files);
}else
diff -up libcdio-0.82/src/cdda-player.c.sprintf libcdio-0.82/src/cdda-player.c
--- libcdio-0.82/src/cdda-player.c.sprintf 2011-05-19 08:54:48.151672692
+0200
+++ libcdio-0.82/src/cdda-player.c 2011-05-19 08:59:29.486672620 +0200
@@ -307,7 +307,7 @@ xperror(const char *psz_msg)
}
if (b_verbose) {
- sprintf(line,"%s: %s", psz_msg, strerror(errno));
+ snprintf(line, 80, "%s: %s", psz_msg, strerror(errno));
attron(A_STANDOUT);
mvprintw(LINE_ACTION, 0, (char *) "error : %s", line);
attroff(A_STANDOUT);
@@ -715,7 +715,7 @@ display_status(bool b_status_only)
if (!b_interactive) return;
if (!b_cd) {
- sprintf(line,"no CD in drive (%s)", psz_device);
+ snprintf(line, 80, "no CD in drive (%s)", psz_device);
} else if (i_first_track == CDIO_CDROM_LEADOUT_TRACK) {
sprintf(line,"CD has only data tracks");
@@ -725,7 +725,7 @@ display_status(bool b_status_only)
cdio_audio_get_volume(p_cdio, &audio_volume);
if (i_vol_port < 4) {
i_volume_level = rounded_div(audio_volume.level[i_vol_port]*100, 256);
- sprintf(line,
+ snprintf(line, 80,
"track %2d - %02x:%02x of %s (%02x:%02x abs) %s volume: %d",
sub.track, sub.rel_addr.m, sub.rel_addr.s,
cd_info[sub.track].length,
@@ -733,12 +733,12 @@ display_status(bool b_status_only)
mmc_audio_state2str(sub.audio_status),
i_volume_level);
} else
- sprintf(line,"track %2d - %02x:%02x of %s (%02x:%02x abs) %s",
+ snprintf(line, 80, "track %2d - %02x:%02x of %s (%02x:%02x abs) %s",
sub.track, sub.rel_addr.m, sub.rel_addr.s,
cd_info[sub.track].length, sub.abs_addr.m, sub.abs_addr.s,
mmc_audio_state2str(sub.audio_status));
} else {
- sprintf(line,"%s", mmc_audio_state2str(sub.audio_status));
+ snprintf(line, 80, "%s", mmc_audio_state2str(sub.audio_status));
}
@@ -979,7 +979,7 @@ display_tracks(void)
s = cdio_audio_get_msf_seconds(&toc[i+1])
- cdio_audio_get_msf_seconds(&toc[i]);
read_subchannel(p_cdio);
- sprintf(line, "%2d %02d:%02d %s ", i,
+ snprintf(line, 200, "%2d %02d:%02d %s ", i,
s / CDIO_CD_SECS_PER_MIN, s % CDIO_CD_SECS_PER_MIN,
( ( sub.audio_status == CDIO_MMC_READ_SUB_ST_PLAY ||
sub.audio_status == CDIO_MMC_READ_SUB_ST_PAUSED ) &&
diff -up libcdio-0.82/example/audio.c.staticanal libcdio-0.82/example/audio.c
--- libcdio-0.82/example/audio.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/example/audio.c 2011-05-19 15:01:08.687667113 +0200
@@ -340,6 +340,7 @@ main(int argc, char *argv[])
i_volume_level = atoi(optarg);
todo = SET_VOLUME;
}
+ break;
case 't':
if (NULL != (h = strchr(optarg,'-'))) {
*h = 0;
diff -up libcdio-0.82/example/mmc2a.c.staticanal libcdio-0.82/example/mmc2a.c
--- libcdio-0.82/example/mmc2a.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/example/mmc2a.c 2011-05-19 15:01:08.687667113 +0200
@@ -41,7 +41,7 @@
static void
print_mode_sense (const char *psz_drive, const char *six_or_ten,
- const uint8_t buf[22])
+ const uint8_t buf[30])
{
printf("Mode sense %s information for %s:\n", six_or_ten, psz_drive);
if (buf[2] & 0x01) {
@@ -211,7 +211,7 @@ main(int argc, const char *argv[])
printf("Couldn't find CD\n");
return 1;
} else {
- uint8_t buf[22] = { 0, }; /* Place to hold returned data */
+ uint8_t buf[30] = { 0, }; /* Place to hold returned data */
char *psz_cd = cdio_get_default_device(p_cdio);
if (DRIVER_OP_SUCCESS == mmc_mode_sense_6(p_cdio, buf, sizeof(buf),
CDIO_MMC_CAPABILITIES_PAGE) ) {
diff -up libcdio-0.82/example/paranoia.c.staticanal
libcdio-0.82/example/paranoia.c
--- libcdio-0.82/example/paranoia.c.staticanal 2009-04-20 13:03:15.000000000
+0200
+++ libcdio-0.82/example/paranoia.c 2011-05-19 15:01:08.689667113 +0200
@@ -129,6 +129,10 @@ main(int argc, const char *argv[])
track_t i_track = cdda_sector_gettrack(d, i_first_lsn);
lsn_t i_last_lsn = cdda_track_lastsector(d, i_track);
int fd = creat("track1s.wav", 0644);
+ if (-1 == fd) {
+ printf("Unable to create track1s.wav\n");
+ exit(1);
+ }
/* For demo purposes we'll read only 300 frames (about 4
seconds). We don't want this to take too long. On the other
diff -up libcdio-0.82/lib/cdda_interface/cddap_interface.c.staticanal
libcdio-0.82/lib/cdda_interface/cddap_interface.c
--- libcdio-0.82/lib/cdda_interface/cddap_interface.c.staticanal
2008-11-29 05:52:43.000000000 +0100
+++ libcdio-0.82/lib/cdda_interface/cddap_interface.c 2011-05-19
15:01:08.690667113 +0200
@@ -281,6 +281,7 @@ verify_read_command(cdrom_drive_t *d)
if(!audioflag){
cdmessage(d,"\tCould not find any audio tracks on this disk.\n");
+ free(buff);
return(-403);
}
diff -up libcdio-0.82/lib/cdda_interface/common_interface.c.staticanal
libcdio-0.82/lib/cdda_interface/common_interface.c
--- libcdio-0.82/lib/cdda_interface/common_interface.c.staticanal
2008-11-29 05:52:43.000000000 +0100
+++ libcdio-0.82/lib/cdda_interface/common_interface.c 2011-05-19
15:01:08.691667113 +0200
@@ -58,8 +58,8 @@ data_bigendianp(cdrom_drive_t *d)
float *a=calloc(1024,sizeof(float));
float *b=calloc(1024,sizeof(float));
long readsectors=5;
- int16_t *buff=malloc(readsectors*CDIO_CD_FRAMESIZE_RAW);
- memset(buff, 0, readsectors*CDIO_CD_FRAMESIZE_RAW);
+ int16_t *buff=malloc(readsectors*CDIO_CD_FRAMESIZE_RAW*sizeof(int16_t));
+ memset(buff, 0, readsectors*CDIO_CD_FRAMESIZE_RAW*sizeof(int16_t));
/* look at the starts of the audio tracks */
/* if real silence, tool in until some static is found */
diff -up libcdio-0.82/lib/cdda_interface/scan_devices.c.staticanal
libcdio-0.82/lib/cdda_interface/scan_devices.c
--- libcdio-0.82/lib/cdda_interface/scan_devices.c.staticanal 2009-07-03
01:41:48.000000000 +0200
+++ libcdio-0.82/lib/cdda_interface/scan_devices.c 2011-05-19
15:01:08.694667113 +0200
@@ -323,7 +323,6 @@ cdda_identify_device_cdio(CdIo_t *p_cdio
snprintf( d->drive_model, i_len, "%s %s %s %s",
hw_info.psz_vendor, hw_info.psz_model, hw_info.psz_revision,
description );
- free(description);
} else {
d->drive_model=malloc( i_len );
snprintf( d->drive_model, i_len, "%s %s %s",
@@ -335,5 +334,8 @@ cdda_identify_device_cdio(CdIo_t *p_cdio
}
}
+ if (description)
+ free(description);
+
return(d);
}
diff -up libcdio-0.82/lib/driver/device.c.staticanal
libcdio-0.82/lib/driver/device.c
--- libcdio-0.82/lib/driver/device.c.staticanal 2009-07-13 01:21:30.000000000
+0200
+++ libcdio-0.82/lib/driver/device.c 2011-05-19 15:01:08.695667113 +0200
@@ -1044,7 +1044,7 @@ driver_return_code_t
cdio_set_blocksize ( const CdIo_t *p_cdio, int i_blocksize )
{
if (!p_cdio) return DRIVER_OP_UNINIT;
- if (p_cdio->op.set_blocksize) return DRIVER_OP_UNSUPPORTED;
+ if (!p_cdio->op.set_blocksize) return DRIVER_OP_UNSUPPORTED;
return p_cdio->op.set_blocksize(p_cdio->env, i_blocksize);
}
diff -up libcdio-0.82/lib/driver/gnu_linux.c.staticanal
libcdio-0.82/lib/driver/gnu_linux.c
--- libcdio-0.82/lib/driver/gnu_linux.c.staticanal 2009-07-03
01:31:58.000000000 +0200
+++ libcdio-0.82/lib/driver/gnu_linux.c 2011-05-19 15:01:08.697667113 +0200
@@ -194,15 +194,17 @@ check_mounts_linux(const char *mtab)
}
}
}
- if ( strcmp(mnt_type, "iso9660") == 0 ) {
- if (is_cdrom_linux(mnt_dev, mnt_type) > 0) {
- free(mnt_type);
- endmntent(mntfp);
- return mnt_dev;
- }
+ if ( mnt_dev && mnt_dev ) {
+ if ( strcmp(mnt_type, "iso9660") == 0 ) {
+ if (is_cdrom_linux(mnt_dev, mnt_type) > 0) {
+ free(mnt_type);
+ endmntent(mntfp);
+ return mnt_dev;
+ }
+ }
+ free(mnt_dev);
+ free(mnt_type);
}
- free(mnt_dev);
- free(mnt_type);
}
endmntent(mntfp);
}
diff -up libcdio-0.82/lib/driver/image/cdrdao.c.staticanal
libcdio-0.82/lib/driver/image/cdrdao.c
--- libcdio-0.82/lib/driver/image/cdrdao.c.staticanal 2008-11-29
05:52:43.000000000 +0100
+++ libcdio-0.82/lib/driver/image/cdrdao.c 2011-05-19 15:01:08.699667113
+0200
@@ -621,18 +621,18 @@ parse_tocfile (_img_private_t *cd, const
goto format_error;
}
} else if (0 == strcmp ("COPY", psz_keyword)) {
- if (NULL != cd)
+ if (NULL != cd && i >= 0)
cd->tocent[i].flags |= CDIO_TRACK_FLAG_COPY_PERMITTED;
} else if (0 == strcmp ("PRE_EMPHASIS", psz_keyword)) {
- if (NULL != cd)
+ if (NULL != cd && i >= 0)
cd->tocent[i].flags |= CDIO_TRACK_FLAG_PRE_EMPHASIS;
/* TWO_CHANNEL_AUDIO */
} else if (0 == strcmp ("TWO_CHANNEL_AUDIO", psz_keyword)) {
- if (NULL != cd)
+ if (NULL != cd && i >= 0)
cd->tocent[i].flags &= ~CDIO_TRACK_FLAG_FOUR_CHANNEL_AUDIO;
/* FOUR_CHANNEL_AUDIO */
} else if (0 == strcmp ("FOUR_CHANNEL_AUDIO", psz_keyword)) {
- if (NULL != cd)
+ if (NULL != cd && i >= 0)
cd->tocent[i].flags |= CDIO_TRACK_FLAG_FOUR_CHANNEL_AUDIO;
/* ISRC "CCOOOYYSSSSS" */
diff -up libcdio-0.82/lib/driver/image/nrg.c.staticanal
libcdio-0.82/lib/driver/image/nrg.c
--- libcdio-0.82/lib/driver/image/nrg.c.staticanal 2009-07-03
01:42:45.000000000 +0200
+++ libcdio-0.82/lib/driver/image/nrg.c 2011-05-19 15:01:08.701667113 +0200
@@ -182,6 +182,7 @@ parse_nrg (_img_private_t *p_env, const
long unsigned int footer_start;
long unsigned int size;
char *footer_buf = NULL;
+ if (!p_env) return false;
size = cdio_stream_stat (p_env->gen.data_source);
if (-1 == size) return false;
diff -up libcdio-0.82/lib/driver/mmc.c.staticanal libcdio-0.82/lib/driver/mmc.c
--- libcdio-0.82/lib/driver/mmc.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/lib/driver/mmc.c 2011-05-19 15:01:08.703667113 +0200
@@ -537,6 +537,7 @@ mmc_set_blocksize_private ( void *p_env,
memset (&mh, 0, sizeof (mh));
mh.block_desc_length = 0x08;
+ /* while i_blocksize is uint16_t, this expression is always 0 */
mh.block_length_hi = (i_blocksize >> 16) & 0xff;
mh.block_length_med = (i_blocksize >> 8) & 0xff;
mh.block_length_lo = (i_blocksize >> 0) & 0xff;
diff -up libcdio-0.82/lib/driver/read.c.staticanal
libcdio-0.82/lib/driver/read.c
--- libcdio-0.82/lib/driver/read.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/lib/driver/read.c 2011-05-19 15:01:08.704667113 +0200
@@ -176,7 +176,7 @@ cdio_read_mode1_sector (const CdIo_t *p_
if (p_cdio->op.read_mode1_sector) {
return p_cdio->op.read_mode1_sector(p_cdio->env, p_buf, i_lsn, b_form2);
} else if (p_cdio->op.lseek && p_cdio->op.read) {
- char buf[CDIO_CD_FRAMESIZE] = { 0, };
+ char buf[M2RAW_SECTOR_SIZE] = { 0, };
if (0 > cdio_lseek(p_cdio, CDIO_CD_FRAMESIZE*i_lsn, SEEK_SET))
return -1;
if (0 > cdio_read(p_cdio, buf, CDIO_CD_FRAMESIZE))
diff -up libcdio-0.82/lib/iso9660/iso9660_fs.c.staticanal
libcdio-0.82/lib/iso9660/iso9660_fs.c
--- libcdio-0.82/lib/iso9660/iso9660_fs.c.staticanal 2008-11-29
05:52:43.000000000 +0100
+++ libcdio-0.82/lib/iso9660/iso9660_fs.c 2011-05-19 15:01:08.706667113
+0200
@@ -192,10 +192,11 @@ iso9660_open_ext_private (const char *ps
return p_iso;
error:
- if (p_iso && p_iso->stream) {
+ if (p_iso->stream)
cdio_stdio_destroy(p_iso->stream);
- free(p_iso);
- }
+
+ free(p_iso);
+
return NULL;
}
@@ -1029,6 +1030,7 @@ _fs_stat_traverse (const CdIo_t *p_cdio,
if (!trans_fname) {
cdio_warn("can't allocate %lu bytes",
(long unsigned int) strlen(p_stat->filename));
+ free(p_stat);
return NULL;
}
trans_len = iso9660_name_translate_ext(p_stat->filename, trans_fname,
@@ -1135,6 +1137,7 @@ _fs_iso_stat_traverse (iso9660_t *p_iso,
if (!trans_fname) {
cdio_warn("can't allocate %lu bytes",
(long unsigned int) strlen(p_stat->filename));
+ free(p_stat);
return NULL;
}
trans_len = iso9660_name_translate_ext(p_stat->filename, trans_fname,
diff -up libcdio-0.82/lib/paranoia/paranoia.c.staticanal
libcdio-0.82/lib/paranoia/paranoia.c
--- libcdio-0.82/lib/paranoia/paranoia.c.staticanal 2008-11-29
05:52:43.000000000 +0100
+++ libcdio-0.82/lib/paranoia/paranoia.c 2011-05-19 15:01:08.709667113
+0200
@@ -1357,14 +1357,14 @@ i_stage2_each(root_block *root, v_fragme
void(*callback)(long int, paranoia_cb_mode_t))
{
+ /* If this fragment has already been merged & freed, abort. */
+ if (!v || !v->one) return(0);
+
cdrom_paranoia_t *p=v->p;
/* ??? Why do we round down to an even dynoverlap? */
long dynoverlap=p->dynoverlap/2*2;
- /* If this fragment has already been merged & freed, abort. */
- if (!v || !v->one) return(0);
-
/* If there's no verified root yet, abort. */
if (!rv(root)){
return(0);
diff -up libcdio-0.82/lib/udf/udf_file.c.staticanal
libcdio-0.82/lib/udf/udf_file.c
--- libcdio-0.82/lib/udf/udf_file.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/lib/udf/udf_file.c 2011-05-19 15:01:08.712667113 +0200
@@ -192,9 +192,10 @@ offset_to_lba(const udf_dirent_t *p_udf_
*/
*pi_max_size = 0;
printf("Don't know how to data in ICB handle yet\n");
-
+ break;
case ICBTAG_FLAG_AD_EXTENDED:
printf("Don't know how to handle extended addresses yet\n");
+ break;
default:
printf("Unsupported allocation descriptor %d\n", addr_ilk);
return CDIO_INVALID_LBA;
diff -up libcdio-0.82/lib/udf/udf_fs.c.staticanal libcdio-0.82/lib/udf/udf_fs.c
--- libcdio-0.82/lib/udf/udf_fs.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/lib/udf/udf_fs.c 2011-05-19 15:01:08.713667113 +0200
@@ -658,8 +658,9 @@ udf_readdir(udf_dirent_t *p_udf_dirent)
uint8_t data[UDF_BLOCKSIZE] = {0};
udf_file_entry_t *p_udf_fe = (udf_file_entry_t *) &data;
- udf_read_sectors(p_udf, p_udf_fe, p_udf->i_part_start
- + p_udf_dirent->fid->icb.loc.lba, 1);
+ if (DRIVER_OP_SUCCESS != udf_read_sectors(p_udf, p_udf_fe,
p_udf->i_part_start
+ + p_udf_dirent->fid->icb.loc.lba, 1))
+ return NULL;
memcpy(&(p_udf_dirent->fe), p_udf_fe,
sizeof(udf_file_entry_t) + p_udf_fe->i_alloc_descs
diff -up libcdio-0.82/src/cd-info.c.staticanal libcdio-0.82/src/cd-info.c
--- libcdio-0.82/src/cd-info.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/src/cd-info.c 2011-05-19 15:01:08.715667113 +0200
@@ -530,6 +530,8 @@ print_iso9660_recurse (CdIo_t *p_cdio, c
if (NULL == p_entlist) {
report( stderr, "Error getting above directory information\n" );
+ free(translated_name);
+ free(p_dirlist);
return;
}
diff -up libcdio-0.82/src/cd-paranoia/cd-paranoia.c.staticanal
libcdio-0.82/src/cd-paranoia/cd-paranoia.c
--- libcdio-0.82/src/cd-paranoia/cd-paranoia.c.staticanal 2009-07-12
01:40:25.000000000 +0200
+++ libcdio-0.82/src/cd-paranoia/cd-paranoia.c 2011-05-19 15:01:08.717667113
+0200
@@ -342,7 +342,7 @@ callback(long int inpos, paranoia_cb_mod
if (callscript)
fprintf(stderr, "##: %d [%s] @ %ld\n",
- function, ((int) function >= -2 && (int) function <= 13 ?
+ function, ((int) function >= -2 && (int) function < 13 ?
callback_strings[function+2] : ""),
inpos);
@@ -1138,6 +1138,11 @@ main(int argc,char *argv[])
if (optind+1<argc) {
if (!strcmp(argv[optind+1],"-") ){
out = dup(fileno(stdout));
+ if(out==-1){
+ report2("Cannot dupplicate stdout: %s",
+ strerror(errno));
+ exit(1);
+ }
if(batch)
report("Are you sure you wanted 'batch' "
"(-B) output with stdout?");
diff -up libcdio-0.82/src/cd-read.c.staticanal libcdio-0.82/src/cd-read.c
--- libcdio-0.82/src/cd-read.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/src/cd-read.c 2011-05-19 15:01:08.718667113 +0200
@@ -546,10 +546,13 @@ main(int argc, char *argv[])
break;
case READ_M1F2:
blocklen=M2RAW_SECTOR_SIZE;
+ break;
case READ_M2F1:
blocklen=CDIO_CD_FRAMESIZE;
+ break;
case READ_M2F2:
blocklen=M2F2_SECTOR_SIZE;
+ break;
default: ;
}
}
diff -up libcdio-0.82/src/iso-info.c.staticanal libcdio-0.82/src/iso-info.c
--- libcdio-0.82/src/iso-info.c.staticanal 2008-11-29 05:52:43.000000000
+0100
+++ libcdio-0.82/src/iso-info.c 2011-05-19 15:02:17.912667096 +0200
@@ -209,6 +209,8 @@ print_iso9660_recurse (iso9660_t *p_iso,
}
if (NULL == entlist) {
+ free(translated_name);
+ free(dirlist);
report( stderr, "Error getting above directory information\n" );
return;
}
