# HG changeset patch # User rubidium # Date 2008-11-23 13:42:05 # Node ID 36d7a611e1f568a2a8ec16da19f14caf3d487947 # Parent 8e9ac02d839a38749377bfc4354f4bb246408b7d (svn r14610) -Fix [FS#2415]: possible stack corruption when reading corrupted sprites. -Change: harden the sprite reading routine against corrupt sprites. diff --git a/src/lang/english.txt b/src/lang/english.txt --- a/src/lang/english.txt +++ b/src/lang/english.txt @@ -3234,6 +3234,7 @@ STR_NEWGRF_ERROR_STATIC_GRF_CAUSES_DESYN STR_NEWGRF_ERROR_UNEXPECTED_SPRITE :Unexpected sprite. STR_NEWGRF_ERROR_UNKNOWN_PROPERTY :Unknown Action 0 property. STR_NEWGRF_ERROR_INVALID_ID :Attempt to use invalid ID. +STR_NEWGRF_ERROR_CORRUPT_SPRITE :{YELLOW}{RAW_STRING} contains a corrupt sprite. All corrupt sprites will be shown as a red question mark (?). STR_NEWGRF_PRESET_LIST_TIP :{BLACK}Load the selected preset STR_NEWGRF_PRESET_SAVE :{BLACK}Save preset diff --git a/src/spritecache.cpp b/src/spritecache.cpp --- a/src/spritecache.cpp +++ b/src/spritecache.cpp @@ -271,7 +271,10 @@ static void* ReadSprite(SpriteCache *sc, sc->type = sprite_type; - if (!sprite_loader.LoadSprite(&sprite, file_slot, file_pos, sprite_type)) return NULL; + if (!sprite_loader.LoadSprite(&sprite, file_slot, file_pos, sprite_type)) { + if (id == SPR_IMG_QUERY) usererror("Okay... something went horribly wrong. I couldn't load the fallback sprite. What should I do?"); + return (void*)GetRawSprite(SPR_IMG_QUERY, ST_NORMAL); + } sc->ptr = BlitterFactoryBase::GetCurrentBlitter()->Encode(&sprite, &AllocSprite); free(sprite.data); diff --git a/src/spriteloader/grf.cpp b/src/spriteloader/grf.cpp --- a/src/spriteloader/grf.cpp +++ b/src/spriteloader/grf.cpp @@ -7,8 +7,31 @@ #include "../fileio_func.h" #include "../debug.h" #include "../core/alloc_func.hpp" +#include "../strings_func.h" +#include "table/strings.h" +#include "../gui.h" #include "grf.hpp" +/** + * We found a corrupted sprite. This means that the sprite itself + * contains invalid data or is too small for the given dimensions. + * @param file_slot the file the errored sprite is in + * @param file_pos the location in the file of the errored sprite + * @param line the line where the error occurs. + * @return always false (to tell loading the sprite failed) + */ +static bool WarnCorruptSprite(uint8 file_slot, size_t file_pos, int line) +{ + static byte warning_level = 0; + if (warning_level == 0) { + SetDParamStr(0, FioGetFilename(file_slot)); + ShowErrorMessage(INVALID_STRING_ID, STR_NEWGRF_ERROR_CORRUPT_SPRITE, 0, 0); + } + DEBUG(sprite, warning_level, "[%i] Loading corrupted sprite from %s at position %i", line, FioGetFilename(file_slot), (int)file_pos); + warning_level = 6; + return false; +} + bool SpriteLoaderGrf::LoadSprite(SpriteLoader::Sprite *sprite, uint8 file_slot, size_t file_pos, SpriteType sprite_type) { /* Open the right file and go to the correct position */ @@ -32,6 +55,7 @@ bool SpriteLoaderGrf::LoadSprite(SpriteL byte *dest_orig = AllocaM(byte, num); byte *dest = dest_orig; + const int dest_size = num; /* Read the file, which has some kind of compression */ while (num > 0) { @@ -41,6 +65,7 @@ bool SpriteLoaderGrf::LoadSprite(SpriteL /* Plain bytes to read */ int size = (code == 0) ? 0x80 : code; num -= size; + if (num < 0) return WarnCorruptSprite(file_slot, file_pos, __LINE__); for (; size > 0; size--) { *dest = FioReadByte(); dest++; @@ -48,8 +73,10 @@ bool SpriteLoaderGrf::LoadSprite(SpriteL } else { /* Copy bytes from earlier in the sprite */ const uint data_offset = ((code & 7) << 8) | FioReadByte(); + if (dest - data_offset < dest_orig) return WarnCorruptSprite(file_slot, file_pos, __LINE__); int size = -(code >> 3); num -= size; + if (num < 0) return WarnCorruptSprite(file_slot, file_pos, __LINE__); for (; size > 0; size--) { *dest = *(dest - data_offset); dest++; @@ -57,7 +84,7 @@ bool SpriteLoaderGrf::LoadSprite(SpriteL } } - assert(num == 0); + if (num != 0) return WarnCorruptSprite(file_slot, file_pos, __LINE__); sprite->data = CallocT(sprite->width * sprite->height); @@ -67,10 +94,16 @@ bool SpriteLoaderGrf::LoadSprite(SpriteL bool last_item = false; /* Look up in the header-table where the real data is stored for this row */ int offset = (dest_orig[y * 2 + 1] << 8) | dest_orig[y * 2]; + /* Go to that row */ - dest = &dest_orig[offset]; + dest = dest_orig + offset; do { + if (dest + 2 > dest_orig + dest_size) { + free(sprite->data); + return WarnCorruptSprite(file_slot, file_pos, __LINE__); + } + SpriteLoader::CommonPixel *data; /* Read the header: * 0 .. 14 - length @@ -82,6 +115,11 @@ bool SpriteLoaderGrf::LoadSprite(SpriteL data = &sprite->data[y * sprite->width + skip]; + if (skip + length > sprite->width || dest + length > dest_orig + dest_size) { + free(sprite->data); + return WarnCorruptSprite(file_slot, file_pos, __LINE__); + } + for (int x = 0; x < length; x++) { data->m = ((sprite_type == ST_NORMAL && _palette_remap_grf[file_slot]) ? _palette_remap[*dest] : *dest); dest++; @@ -90,6 +128,17 @@ bool SpriteLoaderGrf::LoadSprite(SpriteL } while (!last_item); } } else { + if (dest_size < sprite->width * sprite->height) { + free(sprite->data); + return WarnCorruptSprite(file_slot, file_pos, __LINE__); + } + + if (dest_size > sprite->width * sprite->height) { + static byte warning_level = 0; + DEBUG(sprite, warning_level, "Ignoring %i unused extra bytes from the sprite from %s at position %i", dest_size - sprite->width * sprite->height, FioGetFilename(file_slot), (int)file_pos); + warning_level = 6; + } + dest = dest_orig; for (int i = 0; i < sprite->width * sprite->height; i++) {