Cool_Tools/vgmstream
1
0
Fork 0
mirror of https://github.com/vgmstream/vgmstream.git synced 2025-01-17 23:36:41 +01:00
Code Issues Packages Projects Releases Wiki Activity

Fix various leaks and issues found with drmemory

Browse Source
This commit is contained in:
bnnm 2019-09-29 18:25:24 +02:00
parent 2b2b9e0ec8
commit 1b34ef1f01
12 changed files with 91 additions and 69 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
doc/DEV.md
View File

@ -22,10 +22,24 @@ There are no hard coding rules but for consistency one could follow the style us
But other styles may be found, this isn't very important as most files are isolated.
Some of the code may be a bit inefficient or duplicated at places, but it isn't much of a problem if gives clarity. vgmstream's performance is fast enough (as it mainly deals with playing songs in real time) so that favors clarity over optimization. Similarly, some code may segfault or even cause infinite loops on bad data, but it's fixed as encountered rather than worrying too much about improbable cases.
### Code quality
There is quite a bit of code that could be improved overall, but given how niche the project is priority is given to adding and improving formats. Some of the code can be inefficient or duplicated at places, but it isn't that much of a problem if gives clarity. vgmstream's performance is fast enough (as it mainly deals with playing songs in real time) so that favors clarity over optimization.
Similarly, parts may segfault or even cause infinite loops on bad data, but it's fixed as encountered rather than worrying too much about improbable cases. There isn't an automated test suite at the moment, so tests are manually done as needed.
For regression testing there is a simple script that compares output of a previous version of vgmstream_cli with current. Some bugs may drastically change output when fixed (for example adjusting loops or decoding) so it could be hard to automate and maintain.
Code is checked for leaks at times using detection tools, but most of vgmstream formats are quite simple and don't need to manage memory. It's mainly useful for files using external decoders or complex segmented/layered layout combos.
```
# recommended to compile with debug info, for example:
make vgmstream_cli EXTRA_CFLAGS="-g" STRIP=echo
# find leaks
drmemory -- vgmstream_cli -o file.ext
```
## Structure
## Source structure
```
./ scripts

3
src/coding/atrac9_decoder.c
View File

@ -49,7 +49,8 @@ atrac9_codec_data *init_atrac9(atrac9_config *cfg) {
/* must hold at least one superframe and its samples */
data->data_buffer_size = data->info.superframeSize;
data->data_buffer = calloc(sizeof(uint8_t), data->data_buffer_size);
/* extra leeway as Atrac9Decode seems to overread ~2 bytes (doesn't affect decoding though) */
data->data_buffer = calloc(sizeof(uint8_t), data->data_buffer_size + 0x10);
data->sample_buffer = calloc(sizeof(sample_t), data->info.channels * data->info.frameSamples * data->info.framesInSuperframe);
data->samples_to_discard = cfg->encoder_delay;

33
src/coding/ffmpeg_decoder.c
View File

@ -256,11 +256,6 @@ static int ffmpeg_read(void *opaque, uint8_t *buf, int read_size) {
return bytes + max_to_copy;
}
/* AVIO callback: write stream not needed */
static int ffmpeg_write(void *opaque, uint8_t *buf, int buf_size) {
return -1;
}
/* AVIO callback: seek stream, handling custom data */
static int64_t ffmpeg_seek(void *opaque, int64_t offset, int whence) {
ffmpeg_codec_data *data = (ffmpeg_codec_data *) opaque;
@ -488,7 +483,7 @@ static int init_ffmpeg_config(ffmpeg_codec_data * data, int target_subsong, int
data->buffer = av_malloc(FFMPEG_DEFAULT_IO_BUFFER_SIZE);
if (!data->buffer) goto fail;
data->ioCtx = avio_alloc_context(data->buffer, FFMPEG_DEFAULT_IO_BUFFER_SIZE, 0, data, ffmpeg_read, ffmpeg_write, ffmpeg_seek);
data->ioCtx = avio_alloc_context(data->buffer, FFMPEG_DEFAULT_IO_BUFFER_SIZE, 0, data, ffmpeg_read, 0, ffmpeg_seek);
if (!data->ioCtx) goto fail;
data->formatCtx = avformat_alloc_context();
@ -496,8 +491,10 @@ static int init_ffmpeg_config(ffmpeg_codec_data * data, int target_subsong, int
data->formatCtx->pb = data->ioCtx;
//on reset could use AVFormatContext.iformat to reload old format
errcode = avformat_open_input(&data->formatCtx, "", NULL, NULL);
//data->inputFormatCtx = av_find_input_format("h264"); /* set directly? */
/* on reset could use AVFormatContext.iformat to reload old format too */
errcode = avformat_open_input(&data->formatCtx, NULL /*""*/, NULL, NULL);
if (errcode < 0) goto fail;
errcode = avformat_find_stream_info(data->formatCtx, NULL);
@ -554,9 +551,10 @@ static int init_ffmpeg_config(ffmpeg_codec_data * data, int target_subsong, int
if (!data->lastDecodedFrame) goto fail;
av_frame_unref(data->lastDecodedFrame);
data->lastReadPacket = malloc(sizeof(AVPacket));
data->lastReadPacket = av_malloc(sizeof(AVPacket)); /* av_packet_alloc? */
if (!data->lastReadPacket) goto fail;
av_new_packet(data->lastReadPacket, 0);
//av_packet_unref?
return 0;
fail:
@ -823,33 +821,38 @@ static void free_ffmpeg_config(ffmpeg_codec_data *data) {
if (data->lastReadPacket) {
av_packet_unref(data->lastReadPacket);
free(data->lastReadPacket);
av_free(data->lastReadPacket);
data->lastReadPacket = NULL;
}
if (data->lastDecodedFrame) {
av_frame_unref(data->lastDecodedFrame);
av_free(data->lastDecodedFrame);
data->lastDecodedFrame = NULL;
}
if (data->codecCtx) {
avcodec_close(data->codecCtx);
avcodec_free_context(&(data->codecCtx));
avcodec_free_context(&data->codecCtx);
data->codecCtx = NULL;
}
if (data->formatCtx) {
avformat_close_input(&(data->formatCtx));
avformat_close_input(&data->formatCtx);
//avformat_free_context(data->formatCtx); /* done in close_input */
data->formatCtx = NULL;
}
if (data->ioCtx) {
// buffer passed in is occasionally freed and replaced.
// the replacement must be freed as well.
/* buffer passed in is occasionally freed and replaced.
// the replacement must be free'd as well (below) */
data->buffer = data->ioCtx->buffer;
av_free(data->ioCtx);
avio_context_free(&data->ioCtx);
//av_free(data->ioCtx); /* done in context_free (same thing) */
data->ioCtx = NULL;
}
if (data->buffer) {
av_free(data->buffer);
data->buffer = NULL;
}
//todo avformat_find_stream_info may cause some Win Handle leaks? related to certain option (not happening in gcc builds)
}
void free_ffmpeg(ffmpeg_codec_data *data) {

4
src/coding/ffmpeg_decoder_utils.c
View File

@ -71,6 +71,7 @@ ffmpeg_codec_data * init_ffmpeg_atrac3_raw(STREAMFILE *sf, off_t offset, size_t
return ffmpeg_data;
fail:
free_ffmpeg(ffmpeg_data);
return NULL;
}
@ -181,9 +182,8 @@ ffmpeg_codec_data * init_ffmpeg_atrac3_riff(STREAMFILE *sf, off_t offset, int* o
return ffmpeg_data;
fail:
free_ffmpeg(ffmpeg_data);
return NULL;
}
#endif

7
src/coding/vorbis_custom_decoder.c
View File

@ -195,10 +195,11 @@ void free_vorbis_custom(vorbis_custom_codec_data * data) {
if (!data)
return;
/* internal decoder cleanp */
vorbis_info_clear(&data->vi);
vorbis_comment_clear(&data->vc);
/* internal decoder cleanup */
vorbis_block_clear(&data->vb);
vorbis_dsp_clear(&data->vd);
vorbis_comment_clear(&data->vc);
vorbis_info_clear(&data->vi);
free(data->buffer);
free(data);

1
src/layout/layered.c
View File

@ -152,6 +152,7 @@ void free_layout_layered(layered_layout_data *data) {
}
free(data->layers);
}
free(data->buffer);
free(data);
}

1
src/layout/segmented.c
View File

@ -224,6 +224,7 @@ void free_layout_segmented(segmented_layout_data *data) {
}
free(data->segments);
}
free(data->buffer);
free(data);
}

54
src/meta/riff.c
View File

@ -552,7 +552,33 @@ VGMSTREAM * init_vgmstream_riff(STREAMFILE *streamFile) {
vgmstream->sample_rate = fmt.sample_rate;
vgmstream->channel_layout = fmt.channel_layout;
/* init, samples */
/* coding, layout, interleave */
vgmstream->coding_type = fmt.coding_type;
switch (fmt.coding_type) {
case coding_MSADPCM:
case coding_MS_IMA:
case coding_AICA:
case coding_XBOX_IMA:
case coding_IMA:
#ifdef VGM_USE_FFMPEG
case coding_FFmpeg:
#endif
#ifdef VGM_USE_MAIATRAC3PLUS
case coding_AT3plus:
#endif
#ifdef VGM_USE_ATRAC9
case coding_ATRAC9:
#endif
vgmstream->layout_type = layout_none;
vgmstream->interleave_block_size = fmt.block_size;
break;
default:
vgmstream->layout_type = layout_interleave;
vgmstream->interleave_block_size = fmt.interleave;
break;
}
/* samples, codec init (after setting coding to ensure proper close on failure) */
switch (fmt.coding_type) {
case coding_PCM16LE:
vgmstream->num_samples = pcm_bytes_to_samples(data_size, fmt.channel_count, 16);
@ -672,32 +698,6 @@ VGMSTREAM * init_vgmstream_riff(STREAMFILE *streamFile) {
goto fail;
}
/* coding, layout, interleave */
vgmstream->coding_type = fmt.coding_type;
switch (fmt.coding_type) {
case coding_MSADPCM:
case coding_MS_IMA:
case coding_AICA:
case coding_XBOX_IMA:
case coding_IMA:
#ifdef VGM_USE_FFMPEG
case coding_FFmpeg:
#endif
#ifdef VGM_USE_MAIATRAC3PLUS
case coding_AT3plus:
#endif
#ifdef VGM_USE_ATRAC9
case coding_ATRAC9:
#endif
vgmstream->layout_type = layout_none;
vgmstream->interleave_block_size = fmt.block_size;
break;
default:
vgmstream->layout_type = layout_interleave;
vgmstream->interleave_block_size = fmt.interleave;
break;
}
/* Dynasty Warriors 5 (Xbox) 6ch interleaves stereo frames, probably not official */
if (vgmstream->coding_type == coding_XBOX_IMA && vgmstream->channels > 2) {
vgmstream->layout_type = layout_interleave;

12
src/meta/txtp.c
View File

@ -756,14 +756,14 @@ static int get_fade(const char * config, txtp_mix_data *mix, int *out_n) {
char type, separator;
m = sscanf(config, " %d %c%n", &mix->ch_dst, &type, &n);
if (n == 0 || m != 2) goto fail;
if (m != 2 || n == 0) goto fail;
config += n;
tn += n;
if (type == '^') {
/* full definition */
m = sscanf(config, " %lf ~ %lf = %c @%n", &mix->vol_start, &mix->vol_end, &mix->shape, &n);
if (n == 0 || m != 3) goto fail;
if (m != 3 || n == 0) goto fail;
config += n;
tn += n;
@ -773,7 +773,7 @@ static int get_fade(const char * config, txtp_mix_data *mix, int *out_n) {
tn += n;
m = sscanf(config, " %c%n", &separator, &n);
if (n == 0 || m != 1 || separator != '~') goto fail;
if ( m != 1 || n == 0 || separator != '~') goto fail;
config += n;
tn += n;
@ -783,7 +783,7 @@ static int get_fade(const char * config, txtp_mix_data *mix, int *out_n) {
tn += n;
m = sscanf(config, " %c%n", &separator, &n);
if (n == 0 || m != 1 || separator != '+') goto fail;
if (m != 1 || n == 0 || separator != '+') goto fail;
config += n;
tn += n;
@ -793,7 +793,7 @@ static int get_fade(const char * config, txtp_mix_data *mix, int *out_n) {
tn += n;
m = sscanf(config, " %c%n", &separator, &n);
if (n == 0 || m != 1 || separator != '~') goto fail;
if (m != 1 || n == 0 || separator != '~') goto fail;
config += n;
tn += n;
@ -832,7 +832,7 @@ static int get_fade(const char * config, txtp_mix_data *mix, int *out_n) {
tn += n;
m = sscanf(config, " %c%n", &separator, &n);
if (n == 0 || m != 1 || separator != '+') goto fail;
if (m != 1 || n == 0 || separator != '+') goto fail;
config += n;
tn += n;

5
src/meta/xvag.c
View File

@ -307,15 +307,14 @@ static layered_layout_data* build_layered_xvag(STREAMFILE *streamFile, xvag_head
goto fail;
}
if ( !vgmstream_open_stream(data->layers[i], temp_streamFile, 0x00) ) {
if ( !vgmstream_open_stream(data->layers[i], temp_streamFile, 0x00) )
goto fail;
}
close_streamfile(temp_streamFile);
}
/* setup layered VGMSTREAMs */
if (!setup_layout_layered(data))
goto fail;
close_streamfile(temp_streamFile);
return data;
fail:

14
src/streamfile.c
View File

@ -117,18 +117,17 @@ static void close_stdio(STDIOSTREAMFILE * streamfile) {
}
static STREAMFILE *open_stdio(STDIOSTREAMFILE *streamFile,const char * const filename,size_t buffersize) {
int newfd;
FILE *newfile;
STREAMFILE *newstreamFile;
if (!filename)
return NULL;
#if !defined (__ANDROID__)
// if same name, duplicate the file pointer we already have open
if (streamFile->infile && !strcmp(streamFile->name,filename)) {
if (((newfd = dup(fileno(streamFile->infile))) >= 0) &&
(newfile = fdopen( newfd, "rb" )))
{
int newfd;
FILE *newfile;
STREAMFILE *newstreamFile;
if ( ((newfd = dup(fileno(streamFile->infile))) >= 0) && (newfile = fdopen( newfd, "rb")) ) {
newstreamFile = open_stdio_streamfile_buffer_by_file(newfile,filename,buffersize);
if (newstreamFile) {
return newstreamFile;
@ -704,6 +703,7 @@ static STREAMFILE *multifile_open(MULTIFILE_STREAMFILE *streamfile, const char *
new_sf = open_multifile_streamfile(new_inner_sfs, streamfile->inner_sfs_size);
if (!new_sf) goto fail;
free(new_inner_sfs);
return new_sf;
}
else {

8
src/vgmstream.c
View File

@ -2498,7 +2498,7 @@ static void try_dual_file_stereo(VGMSTREAM * opened_vgmstream, STREAMFILE *strea
return;
extension = (char *)filename_extension(new_filename);
if (extension - new_filename >= 1 && extension[-1] == '.')
if (extension - new_filename >= 1 && extension[-1] == '.') /* [-1] is ok, yeah */
extension--; /* must include "." */
extension_len = strlen(extension);
@ -2514,7 +2514,7 @@ static void try_dual_file_stereo(VGMSTREAM * opened_vgmstream, STREAMFILE *strea
//;VGM_LOG("DFS: l=%s, r=%s\n", this_suffix,that_suffix);
/* is suffix matches paste opposite suffix (+ terminator) to extension pointer, thus to new_filename */
/* if suffix matches paste opposite suffix (+ terminator) to extension pointer, thus to new_filename */
if (this_suffix[0] == '.' && extension_len == this_suffix_len) { /* same extension */
//;VGM_LOG("DFS: same ext %s vs %s len %i\n", extension, this_suffix, this_suffix_len);
if (memcmp(extension,this_suffix,this_suffix_len) == 0) {
@ -2522,7 +2522,7 @@ static void try_dual_file_stereo(VGMSTREAM * opened_vgmstream, STREAMFILE *strea
memcpy (extension, that_suffix,that_suffix_len+1);
}
}
else if (filename_len > this_suffix_len) { /* same suffix (without extension) */
else if (filename_len - extension_len > this_suffix_len) { /* same suffix (without extension) */
//;VGM_LOG("DFS: same suf %s vs %s len %i\n", extension - this_suffix_len, this_suffix, this_suffix_len);
if (memcmp(extension - this_suffix_len, this_suffix,this_suffix_len) == 0) {
dfs_pair = j;
@ -2623,6 +2623,8 @@ static void try_dual_file_stereo(VGMSTREAM * opened_vgmstream, STREAMFILE *strea
opened_vgmstream->channels = 2;
/* discard the second VGMSTREAM */
mixing_close(new_vgmstream);
free(new_vgmstream->start_vgmstream);
free(new_vgmstream);
mixing_update_channel(opened_vgmstream); /* notify of new channel hacked-in */