valter/dosbox-staging
1
0
Fork 0
Code Releases Activity

Fix double-free in destructor of CDROM_Interface_Image

Browse source 
Unmounting single binary-file CDROM images would result in a
crash as flagged by @dreamer_ in issue #112, who noted that
multiple tracks point to the same TrackFile (track.file field).

During destruction, the pre-C++11 code used a temporary
`TrackFile* last` variable to store the currently deleted
track.file's value and only delete the next track.file if it's
not equal to `last`. The result of this is that only the
first-encountered track.file was deleted and all subsequent
and back-to-back duplicate track.file values were left as-is.

This commit replaces the Track object's 'file' member pointer
with a shared_ptr, which effectively does the same thing by
only deleting the last reference.

The shared_ptr simplifies some error-cases where we previously
had to delete the Track.file allocation, but can now simply
let the Track object go out-of-scope and let the shared_ptr
delete it's managed object (if it has one).
This commit is contained in:
krcroft 2020-01-05 17:31:31 -08:00 • committed by Patryk Obara
parent a852fe3eab
commit cc7d6b6e43
2 changed files with 27 additions and 70 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

17
src/dos/cdrom.h
View file

@ -23,6 +23,7 @@
#include <cstring>
#include <fstream>
#include <iostream>
#include <memory>
#include <sstream>
#include <string>
#include <vector>
@ -183,14 +184,14 @@ private:
public:
// Nested struct definition
struct Track {
TrackFile *file;
int number;
int attr;
int start;
int length;
int skip;
int sectorSize;
bool mode2;
std::shared_ptr<TrackFile> file = nullptr;
int number = 0;
int attr = 0;
int start = 0;
int length = 0;
int skip = 0;
int sectorSize = 0;
bool mode2 = false;
};
CDROM_Interface_Image (Bit8u _subUnit);
virtual ~CDROM_Interface_Image (void);

80
src/dos/cdrom_image.cpp
View file

@ -463,7 +463,7 @@ bool CDROM_Interface_Image::PlayAudioSector(unsigned long start, unsigned long l
+ clamp(relative_start, 0, track->length - 1)
* track->sectorSize);
TrackFile* trackFile = track->file;
TrackFile* trackFile = track->file.get();
// Guard: Bail if our track could not be seeked
if (!trackFile->seek(offset)) {
LOG_MSG("CDROM: Track %d failed to seek to byte %u, so cancelling playback",
@ -792,47 +792,31 @@ void CDROM_Interface_Image::CDAudioCallBack(Bitu desired_track_frames)
bool CDROM_Interface_Image::LoadIsoFile(char* filename)
{
tracks.clear();
// data track
Track track = {nullptr, // TrackFile*
0, // number
0, // attr
0, // start
0, // length
0, // skip
0, // sectorSize
false}; // mode2
// data track (track 1)
Track track;
bool error;
track.file = new BinaryFile(filename, error);
track.file = make_shared<BinaryFile>(filename, error);
if (error) {
if (track.file != nullptr) {
delete track.file;
track.file = nullptr;
}
return false;
}
track.number = 1;
track.attr = 0x40;//data
// try to detect iso type
if (CanReadPVD(track.file, BYTES_PER_COOKED_REDBOOK_FRAME, false)) {
if (CanReadPVD(track.file.get(), BYTES_PER_COOKED_REDBOOK_FRAME, false)) {
track.sectorSize = BYTES_PER_COOKED_REDBOOK_FRAME;
track.mode2 = false;
} else if (CanReadPVD(track.file, BYTES_PER_RAW_REDBOOK_FRAME, false)) {
} else if (CanReadPVD(track.file.get(), BYTES_PER_RAW_REDBOOK_FRAME, false)) {
track.sectorSize = BYTES_PER_RAW_REDBOOK_FRAME;
track.mode2 = false;
} else if (CanReadPVD(track.file, 2336, true)) {
} else if (CanReadPVD(track.file.get(), 2336, true)) {
track.sectorSize = 2336;
track.mode2 = true;
} else if (CanReadPVD(track.file, BYTES_PER_RAW_REDBOOK_FRAME, true)) {
} else if (CanReadPVD(track.file.get(), BYTES_PER_RAW_REDBOOK_FRAME, true)) {
track.sectorSize = BYTES_PER_RAW_REDBOOK_FRAME;
track.mode2 = true;
} else {
if (track.file != nullptr) {
delete track.file;
track.file = nullptr;
}
return false;
}
track.length = track.file->getLength() / track.sectorSize;
@ -845,13 +829,11 @@ bool CDROM_Interface_Image::LoadIsoFile(char* filename)
tracks.push_back(track);
// leadout track
track.number = 2;
track.attr = 0;
track.start = track.length;
track.length = 0;
track.file = nullptr;
tracks.push_back(track);
// leadout track (track 2)
Track leadout_track;
leadout_track.number = 2;
leadout_track.start = track.length;
tracks.push_back(leadout_track);
return true;
}
@ -891,14 +873,7 @@ static string dirname(char * file) {
bool CDROM_Interface_Image::LoadCueSheet(char *cuefile)
{
Track track = {nullptr, // TrackFile*
0, // number
0, // attr
0, // start
0, // length
0, // skip
0, // sectorSize
false}; // mode2
Track track;
tracks.clear();
int shift = 0;
int currPregap = 0;
@ -985,22 +960,17 @@ bool CDROM_Interface_Image::LoadCueSheet(char *cuefile)
string type;
GetCueKeyword(type, line);
track.file = nullptr;
bool error = true;
if (type == "BINARY") {
track.file = new BinaryFile(filename.c_str(), error);
track.file = make_shared<BinaryFile>(filename.c_str(), error);
}
else {
track.file = new AudioFile(filename.c_str(), error);
track.file = make_shared<AudioFile>(filename.c_str(), error);
// SDL_Sound first tries using a decoder having a matching registered extension
// as the filename, and then falls back to trying each decoder before finally
// giving up.
}
if (error) {
if (track.file != nullptr) {
delete track.file;
track.file = nullptr;
}
success = false;
}
}
@ -1014,17 +984,9 @@ bool CDROM_Interface_Image::LoadCueSheet(char *cuefile)
}
// failure
else {
if (track.file != nullptr) {
delete track.file;
track.file = nullptr;
}
success = false;
}
if (!success) {
if (track.file != nullptr) {
delete track.file;
track.file = nullptr;
}
return false;
}
}
@ -1038,7 +1000,7 @@ bool CDROM_Interface_Image::LoadCueSheet(char *cuefile)
track.attr = 0;//sync with load iso
track.start = 0;
track.length = 0;
track.file = nullptr;
track.file.reset();
if (!AddTrack(track, shift, -1, totalPregap, 0)) {
return false;
}
@ -1212,12 +1174,6 @@ bool CDROM_Interface_Image::GetCueString(string &str, istream &in)
void CDROM_Interface_Image::ClearTracks()
{
for (auto &track : tracks) {
if (track.file) {
delete track.file;
track.file = nullptr;
}
}
tracks.clear();
}