fix incorrect usage of realloc in bitwriter_grow_

[ltx2018]

[petterreinholdtsen]

The patch seem to assume that if realloc() return NULL, the original memory is lost, but the manual page for realloc() at least on Linux state that "If realloc() fails, the original block is left untouched; it is not freed or moved." The C standard states that "If memory for the new object cannot be allocated, the old object is not deallocated and its value is unchanged.".

This make me believe the proposed change is not correct.

[ltx2018]

The patch seem to assume that if realloc() return NULL, the original memory is lost, but the manual page for realloc() at least on Linux state that "If realloc() fails, the original block is left untouched; it is not freed or moved." The C standard states that "If memory for the new object cannot be allocated, the old object is not deallocated and its value is unchanged.".

This make me believe the proposed change is not correct.

if realloc() return NULL, original memory will be free in safe_realloc_

static inline void *safe_realloc_(void *ptr, size_t size)
{
	void *oldptr = ptr;
	void *newptr = realloc(ptr, size);
	if(size > 0 && newptr == 0)
		free(oldptr);
	return newptr;
}

[ltx2018]

similar to this patch, in FLAC__stream_decoder_set_metadata_respond_application(stream_decoder.c, line 752)

if(0 == (decoder->private_->metadata_filter_ids = safe_realloc_mul_2op_(decoder->private_->metadata_filter_ids, decoder->private_->metadata_filter_ids_capacity, /*times*/2))) {
	decoder->protected_->state = FLAC__STREAM_DECODER_MEMORY_ALLOCATION_ERROR;
	return false;
}

in read_metadata_seektable_(stream_decoder.c, line 1654)

/* use realloc since we may pass through here several times (e.g. after seeking) */
if(0 == (decoder->private_->seek_table.data.seek_table.points = safe_realloc_mul_2op_(decoder->private_->seek_table.data.seek_table.points, decoder->private_->seek_table.data.seek_table.num_points, /*times*/sizeof(FLAC__StreamMetadata_SeekPoint)))) {
	decoder->protected_->state = FLAC__STREAM_DECODER_MEMORY_ALLOCATION_ERROR;
	return false;
}

[ltx2018]

the problem is size1 > SIZE_MAX / size2 in safe_realloc_mul_2op_ , if that happens original memory not freed, so maybe free(ptr) should be called before return 0.

static inline void *safe_realloc_mul_2op_(void *ptr, size_t size1, size_t size2)
{
	if(!size1 || !size2)
		return realloc(ptr, 0); /* preserve POSIX realloc(ptr, 0) semantics */
	if(size1 > SIZE_MAX / size2)
		return 0;
	return safe_realloc_(ptr, size1*size2);
}

any idea?

[erikd]

@ltx2018 please do not post screen captures, my screen resolution makes them unreadably small. Instead copy and paste and learn how to use GH markup.

[ltx2018]

@ltx2018 please do not post screen captures, my screen resolution makes them unreadably small. Instead copy and paste and learn how to use GH markup.

emm..updated

[ktmf01]

The problem that the first commit tries to fix appeared today during fuzzing, so it seems this fix is still needed. I will take some time to review it soon.

[ktmf01]

It seems commit d9ae5e9 broke this. Another solution is to add a function safe_realloc_nofree_mul_2op_ for example. I've identified 6 other calls to safe_realloc_ variants that need to be checked

./src/flac/foreign_metadata.c:		foreign_block_t *fb = safe_realloc_muladd2_(fm->blocks, sizeof(foreign_block_t), /*times (*/fm->num_blocks, /*+*/1/*)*/);
./src/flac/encode.c:			if(0 == (x = safe_realloc_muladd2_(m->metadata, sizeof(*m->metadata), /*times (*/m->num_metadata, /*+*/1/*)*/)))
./src/flac/encode.c:			if(0 == (x = safe_realloc_muladd2_(m->needs_delete, sizeof(*m->needs_delete), /*times (*/m->num_metadata, /*+*/1/*)*/)))
./src/libFLAC/metadata_object.c:	FLAC__byte *x = safe_realloc_add_2op_(*entry, length, /*+*/1);
./src/plugin_common/tags.c:		if(0 == (new_entry = safe_realloc_add_4op_(entry->entry, entry->length, /*+*/value_len, /*+*/separator_len, /*+*/1)))
./src/share/utf8/iconvert.c:    	newbuf = safe_realloc_add_2op_(utfbuf, (ob - utfbuf), /*+*/1);

Especially the one in metadata_object.c is interesting.

https://github.com/xiph/flac/blob/afedee12513f9a7204da74e0b7bb6f2ad6189c35/src/libFLAC/metadata_object.c#L94-L106

Before commit d9ae5e9 the entry was indeed not touched, but after it the entry was freed upon realloc failing. For this case a safe_realloc_nofree_mul_2op_ would be best I think.

[ktmf01]

This is fixed with the merge of #419