Skip to content

Commit

Permalink
Fixed|Refactor|Resources|Renderer: Removing assumptions about submode…
Browse files Browse the repository at this point in the history 
…l count

In many places the code makes hidden assumptions about the number of
submodels in the defined 3D models. These changes add additional
checks to ensure that no out-of-bounds access occurs.

Now also checking the DED models' submodel array and not just the
renderer's model data.
  • Loading branch information
@skyjake
skyjake committed Jul 6, 2013
1 parent 711dae8 commit fe529a2
Show file tree
Hide file tree
Showing 8 changed files with 182 additions and 106 deletions.
29 changes: 28 additions & 1 deletion doomsday/client/include/def_data.h
View file
Expand Up @@ -215,7 +215,7 @@ struct ded_model_t
float shadowRadius; // Radius for shadow (0=auto).

typedef std::vector<ded_submodel_t> Submodels;
Submodels sub;
Submodels _sub;

ded_model_t(char const *spriteId = "")
: off(0),
Expand All @@ -238,6 +238,33 @@ struct ded_model_t
interRange[1] = 1;
scale[0] = scale[1] = scale[2] = 1;
}

bool hasSub(unsigned int subnum) const
{
return subnum < _sub.size();
}

unsigned int subCount() const
{
return _sub.size();
}

ded_submodel_t &sub(unsigned int subnum)
{
DENG_ASSERT(hasSub(subnum));
return _sub[subnum];
}

ded_submodel_t const &sub(unsigned int subnum) const
{
DENG_ASSERT(hasSub(subnum));
return _sub[subnum];
}

void appendSub()
{
_sub.push_back(ded_submodel_t());
}
};

typedef struct {
Expand Down
40 changes: 34 additions & 6 deletions doomsday/client/include/resource/models.h
View file
Expand Up @@ -158,7 +158,7 @@ struct ModelDef

/// Submodels.
typedef std::vector<SubmodelDef> Subs;
Subs sub;
Subs _sub;

ModelDef(char const *modelDefId = "")
: state(0),
Expand All @@ -183,21 +183,49 @@ struct ModelDef

SubmodelDef *addSub()
{
sub.push_back(SubmodelDef());
_sub.push_back(SubmodelDef());
ptcOffset.push_back(de::Vector3f());
return &sub.back();
return &_sub.back();
}

void clearSubs()
{
sub.clear();
_sub.clear();
ptcOffset.clear();
}

uint subCount() const
{
return _sub.size();
}

bool testSubFlag(unsigned int subnum, int flag) const
{
if(subnum >= sub.size()) return false;
return sub[subnum].testFlag(flag);
if(!hasSub(subnum)) return false;
return _sub[subnum].testFlag(flag);
}

modelid_t subModelId(unsigned int subnum) const
{
if(!hasSub(subnum)) return NOMODELID;
return _sub[subnum].modelId;
}

SubmodelDef &subModelDef(unsigned int subnum)
{
DENG_ASSERT(hasSub(subnum));
return _sub[subnum];
}

SubmodelDef const &subModelDef(unsigned int subnum) const
{
DENG_ASSERT(hasSub(subnum));
return _sub[subnum];
}

bool hasSub(unsigned int subnum) const
{
return subnum < _sub.size();
}
};

Expand Down
4 changes: 2 additions & 2 deletions doomsday/client/src/def_data.cpp
View file
Expand Up @@ -146,9 +146,9 @@ void DED_Clear(ded_t* ded)
for(uint i = 0; i < ded->models.size(); ++i)
{
ded_model_t* mdl = &ded->models[i];
for(uint j = 0; j < mdl->sub.size(); ++j)
for(uint j = 0; j < mdl->subCount(); ++j)
{
ded_submodel_t* sub = &mdl->sub[j];
ded_submodel_t* sub = &mdl->sub(j);
if(sub->filename) Uri_Delete(sub->filename);
if(sub->skinFilename) Uri_Delete(sub->skinFilename);
if(sub->shinySkin) Uri_Delete(sub->shinySkin);
Expand Down
4 changes: 2 additions & 2 deletions doomsday/client/src/def_main.cpp
View file
Expand Up @@ -1876,13 +1876,13 @@ void Def_PostInit(void)
sprintf(name, "Particle%02i", st->type - PTC_MODEL);

modeldef_t *modef = Models_Definition(name);
if(!modef || modef->sub.empty() || modef->sub[0].modelId == NOMODELID)
if(!modef || modef->subModelId(0) == NOMODELID)
{
st->model = -1;
continue;
}

model_t *mdl = Models_ToModel(modef->sub[0].modelId);
model_t *mdl = Models_ToModel(modef->subModelId(0));
DENG_ASSERT(mdl);

st->model = modef - &modefs[0];
Expand Down
80 changes: 40 additions & 40 deletions doomsday/client/src/def_read.cpp
View file
Expand Up @@ -1412,16 +1412,16 @@ static int DED_ReadData(ded_t* ded, const char* buffer, const char* _sourceFile)
if(bCopyNext)
{
*mdl = *prevModel;
for(uint i = 0; i < mdl->sub.size(); ++i)
for(uint i = 0; i < mdl->subCount(); ++i)
{
if(mdl->sub[i].filename)
mdl->sub[i].filename = Uri_Dup(mdl->sub[i].filename);
if(mdl->sub(i).filename)
mdl->sub(i).filename = Uri_Dup(mdl->sub(i).filename);

if(mdl->sub[i].skinFilename)
mdl->sub[i].skinFilename = Uri_Dup(mdl->sub[i].skinFilename);
if(mdl->sub(i).skinFilename)
mdl->sub(i).skinFilename = Uri_Dup(mdl->sub(i).skinFilename);

if(mdl->sub[i].shinySkin)
mdl->sub[i].shinySkin = Uri_Dup(mdl->sub[i].shinySkin);
if(mdl->sub(i).shinySkin)
mdl->sub(i).shinySkin = Uri_Dup(mdl->sub(i).shinySkin);
}
}
}
Expand Down Expand Up @@ -1454,34 +1454,34 @@ static int DED_ReadData(ded_t* ded, const char* buffer, const char* _sourceFile)
if(ISLABEL("Md2") || ISLABEL("Sub"))
{
// Add another submodel.
if(sub >= mdl->sub.size())
if(sub >= mdl->subCount())
{
mdl->sub.push_back(ded_submodel_t());
mdl->appendSub();
}
DENG_ASSERT(sub < mdl->sub.size());
DENG_ASSERT(sub < mdl->subCount());

FINDBEGIN;
for(;;)
{
READLABEL;
RV_URI("File", &mdl->sub[sub].filename, "Models")
RV_STR("Frame", mdl->sub[sub].frame)
RV_INT("Frame range", mdl->sub[sub].frameRange)
RV_BLENDMODE("Blending mode", mdl->sub[sub].blendMode)
RV_INT("Skin", mdl->sub[sub].skin)
RV_URI("Skin file", &mdl->sub[sub].skinFilename, "Models")
RV_INT("Skin range", mdl->sub[sub].skinRange)
RV_VEC("Offset XYZ", mdl->sub[sub].offset, 3)
RV_FLAGS("Flags", mdl->sub[sub].flags, "df_")
RV_FLT("Transparent", mdl->sub[sub].alpha)
RV_FLT("Parm", mdl->sub[sub].parm)
RV_BYTE("Selskin mask", mdl->sub[sub].selSkinBits[0])
RV_BYTE("Selskin shift", mdl->sub[sub].selSkinBits[1])
RV_NBVEC("Selskins", mdl->sub[sub].selSkins, 8)
RV_URI("Shiny skin", &mdl->sub[sub].shinySkin, "Models")
RV_FLT("Shiny", mdl->sub[sub].shiny)
RV_VEC("Shiny color", mdl->sub[sub].shinyColor, 3)
RV_FLT("Shiny reaction", mdl->sub[sub].shinyReact)
RV_URI("File", &mdl->sub(sub).filename, "Models")
RV_STR("Frame", mdl->sub(sub).frame)
RV_INT("Frame range", mdl->sub(sub).frameRange)
RV_BLENDMODE("Blending mode", mdl->sub(sub).blendMode)
RV_INT("Skin", mdl->sub(sub).skin)
RV_URI("Skin file", &mdl->sub(sub).skinFilename, "Models")
RV_INT("Skin range", mdl->sub(sub).skinRange)
RV_VEC("Offset XYZ", mdl->sub(sub).offset, 3)
RV_FLAGS("Flags", mdl->sub(sub).flags, "df_")
RV_FLT("Transparent", mdl->sub(sub).alpha)
RV_FLT("Parm", mdl->sub(sub).parm)
RV_BYTE("Selskin mask", mdl->sub(sub).selSkinBits[0])
RV_BYTE("Selskin shift", mdl->sub(sub).selSkinBits[1])
RV_NBVEC("Selskins", mdl->sub(sub).selSkins, 8)
RV_URI("Shiny skin", &mdl->sub(sub).shinySkin, "Models")
RV_FLT("Shiny", mdl->sub(sub).shiny)
RV_VEC("Shiny color", mdl->sub(sub).shinyColor, 3)
RV_FLT("Shiny reaction", mdl->sub(sub).shinyReact)
RV_END
CHECKSC;
}
Expand All @@ -1502,28 +1502,28 @@ static int DED_ReadData(ded_t* ded, const char* buffer, const char* _sourceFile)
//if(!strcmp(mdl->group, "-")) strcpy(mdl->group, prevModel->group);
//if(!strcmp(mdl->flags, "-")) strcpy(mdl->flags, prevModel->flags);

for(uint i = 0; i < mdl->sub.size(); ++i)
for(uint i = 0; i < mdl->subCount(); ++i)
{
if(mdl->sub[i].filename && !Str_CompareIgnoreCase(Uri_Path(mdl->sub[i].filename), "-"))
if(mdl->sub(i).filename && !Str_CompareIgnoreCase(Uri_Path(mdl->sub(i).filename), "-"))
{
Uri_Delete(mdl->sub[i].filename);
mdl->sub[i].filename = NULL;
Uri_Delete(mdl->sub(i).filename);
mdl->sub(i).filename = NULL;
}

if(mdl->sub[i].skinFilename && !Str_CompareIgnoreCase(Uri_Path(mdl->sub[i].skinFilename), "-"))
if(mdl->sub(i).skinFilename && !Str_CompareIgnoreCase(Uri_Path(mdl->sub(i).skinFilename), "-"))
{
Uri_Delete(mdl->sub[i].skinFilename);
mdl->sub[i].skinFilename = NULL;
Uri_Delete(mdl->sub(i).skinFilename);
mdl->sub(i).skinFilename = NULL;
}

if(mdl->sub[i].shinySkin && !Str_CompareIgnoreCase(Uri_Path(mdl->sub[i].shinySkin), "-"))
if(mdl->sub(i).shinySkin && !Str_CompareIgnoreCase(Uri_Path(mdl->sub(i).shinySkin), "-"))
{
Uri_Delete(mdl->sub[i].shinySkin);
mdl->sub[i].shinySkin = NULL;
Uri_Delete(mdl->sub(i).shinySkin);
mdl->sub(i).shinySkin = NULL;
}

if(!strcmp(mdl->sub[i].frame, "-"))
memset(mdl->sub[i].frame, 0, sizeof(ded_string_t));
if(!strcmp(mdl->sub(i).frame, "-"))
memset(mdl->sub(i).frame, 0, sizeof(ded_string_t));
}
}

Expand Down

0 comments on commit fe529a2

Please sign in to comment.