6737: Refactor `str_copy` usages and buffer sizes in editor r=def- a=Robyt3

Use templated `str_copy` function in most cases to specify the correct buffer size.

Fixes two cases where the buffer size was hard-coded to a number.

Fix buffer size not being checked when copying auto mapper configuration name. Loading an auto mapper configuration with a name longer than 128 bytes could cause an out-of-bounds write.

Increase sizes of some text buffers that were potentially too small.

## Checklist

- [X] Tested the change ingame
- [ ] Provided screenshots if it is a visual change
- [ ] Tested in combination with possibly related configuration options
- [ ] Written a unit test (especially base/) or added coverage to integration test
- [ ] Considered possible null pointers and out of bounds array indexing
- [ ] Changed no physics that affect existing maps
- [ ] Tested the change with [ASan+UBSan or valgrind's memcheck](https://github.com/ddnet/ddnet/#using-addresssanitizer--undefinedbehavioursanitizer-or-valgrinds-memcheck) (optional)


Co-authored-by: Robert Müller <robytemueller@gmail.com>
This commit is contained in:
bors[bot] 2023-06-13 20:50:34 +00:00 committed by GitHub
commit 183086700f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 52 additions and 53 deletions

View file

@ -45,7 +45,7 @@ CAutoMapper::CAutoMapper(CEditor *pEditor)
void CAutoMapper::Load(const char *pTileName)
{
char aPath[256];
char aPath[IO_MAX_PATH_LENGTH];
str_format(aPath, sizeof(aPath), "editor/%s.rules", pTileName);
IOHANDLE RulesFile = m_pEditor->Storage()->OpenFile(aPath, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL);
if(!RulesFile)
@ -58,8 +58,6 @@ void CAutoMapper::Load(const char *pTileName)
CRun *pCurrentRun = nullptr;
CIndexRule *pCurrentIndex = nullptr;
char aBuf[256];
// read each line
while(char *pLine = LineReader.Get())
{
@ -79,7 +77,7 @@ void CAutoMapper::Load(const char *pTileName)
m_vConfigs.push_back(NewConf);
int ConfigurationID = m_vConfigs.size() - 1;
pCurrentConf = &m_vConfigs[ConfigurationID];
str_copy(pCurrentConf->m_aName, pLine, str_length(pLine));
str_copy(pCurrentConf->m_aName, pLine, minimum<int>(sizeof(pCurrentConf->m_aName), str_length(pLine)));
// add start run
CRun NewRun;
@ -362,8 +360,9 @@ void CAutoMapper::Load(const char *pTileName)
io_close(RulesFile);
char aBuf[IO_MAX_PATH_LENGTH + 16];
str_format(aBuf, sizeof(aBuf), "loaded %s", aPath);
m_pEditor->Console()->Print(IConsole::OUTPUT_LEVEL_DEBUG, "editor", aBuf);
m_pEditor->Console()->Print(IConsole::OUTPUT_LEVEL_DEBUG, "editor/automap", aBuf);
m_FileLoaded = true;
}

View file

@ -844,7 +844,7 @@ bool CEditor::CallbackSaveMap(const char *pFileName, int StorageType, void *pUse
if(pEditor->Save(pFileName))
{
str_copy(pEditor->m_aFileName, pFileName, sizeof(pEditor->m_aFileName));
str_copy(pEditor->m_aFileName, pFileName);
pEditor->m_ValidSaveFilename = StorageType == IStorage::TYPE_SAVE && pEditor->m_pFileDialogPath == pEditor->m_aFileDialogCurrentFolder;
pEditor->m_Map.m_Modified = false;
pEditor->m_Dialog = DIALOG_NONE;
@ -3208,7 +3208,7 @@ int CEditor::DoProperties(CUIRect *pToolBox, CProperty *pProps, int *pIDs, int *
{
char aBuf[64];
if(pProps[i].m_Value < 0)
str_copy(aBuf, "None", sizeof(aBuf));
str_copy(aBuf, "None");
else
str_copy(aBuf, m_Map.m_vpImages[pProps[i].m_Value]->m_aName);
@ -3260,7 +3260,7 @@ int CEditor::DoProperties(CUIRect *pToolBox, CProperty *pProps, int *pIDs, int *
{
char aBuf[64];
if(pProps[i].m_Value < 0)
str_copy(aBuf, "None", sizeof(aBuf));
str_copy(aBuf, "None");
else
str_copy(aBuf, m_Map.m_vpSounds[pProps[i].m_Value]->m_aName);
@ -3279,7 +3279,7 @@ int CEditor::DoProperties(CUIRect *pToolBox, CProperty *pProps, int *pIDs, int *
{
char aBuf[64];
if(pProps[i].m_Value < 0 || pProps[i].m_Min < 0 || pProps[i].m_Min >= (int)m_Map.m_vpImages.size())
str_copy(aBuf, "None", sizeof(aBuf));
str_copy(aBuf, "None");
else
str_copy(aBuf, m_Map.m_vpImages[pProps[i].m_Min]->m_AutoMapper.GetConfigName(pProps[i].m_Value));
@ -3304,7 +3304,7 @@ int CEditor::DoProperties(CUIRect *pToolBox, CProperty *pProps, int *pIDs, int *
Shifter.VSplitLeft(10.0f, &Dec, &Shifter);
if(CurValue <= 0)
str_copy(aBuf, "None:", sizeof(aBuf));
str_copy(aBuf, "None:");
else if(m_Map.m_vpEnvelopes[CurValue - 1]->m_aName[0])
{
str_format(aBuf, sizeof(aBuf), "%s:", m_Map.m_vpEnvelopes[CurValue - 1]->m_aName);
@ -3597,26 +3597,26 @@ void CEditor::RenderLayers(CUIRect LayersBox)
m_Map.m_vpGroups[g]->m_vpLayers[i]->m_Visible = !m_Map.m_vpGroups[g]->m_vpLayers[i]->m_Visible;
if(m_Map.m_vpGroups[g]->m_vpLayers[i]->m_aName[0])
str_copy(aBuf, m_Map.m_vpGroups[g]->m_vpLayers[i]->m_aName, sizeof(aBuf));
str_copy(aBuf, m_Map.m_vpGroups[g]->m_vpLayers[i]->m_aName);
else
{
if(m_Map.m_vpGroups[g]->m_vpLayers[i]->m_Type == LAYERTYPE_TILES)
{
CLayerTiles *pTiles = (CLayerTiles *)m_Map.m_vpGroups[g]->m_vpLayers[i];
str_copy(aBuf, pTiles->m_Image >= 0 ? m_Map.m_vpImages[pTiles->m_Image]->m_aName : "Tiles", sizeof(aBuf));
str_copy(aBuf, pTiles->m_Image >= 0 ? m_Map.m_vpImages[pTiles->m_Image]->m_aName : "Tiles");
}
else if(m_Map.m_vpGroups[g]->m_vpLayers[i]->m_Type == LAYERTYPE_QUADS)
{
CLayerQuads *pQuads = (CLayerQuads *)m_Map.m_vpGroups[g]->m_vpLayers[i];
str_copy(aBuf, pQuads->m_Image >= 0 ? m_Map.m_vpImages[pQuads->m_Image]->m_aName : "Quads", sizeof(aBuf));
str_copy(aBuf, pQuads->m_Image >= 0 ? m_Map.m_vpImages[pQuads->m_Image]->m_aName : "Quads");
}
else if(m_Map.m_vpGroups[g]->m_vpLayers[i]->m_Type == LAYERTYPE_SOUNDS)
{
CLayerSounds *pSounds = (CLayerSounds *)m_Map.m_vpGroups[g]->m_vpLayers[i];
str_copy(aBuf, pSounds->m_Sound >= 0 ? m_Map.m_vpSounds[pSounds->m_Sound]->m_aName : "Sounds", sizeof(aBuf));
str_copy(aBuf, pSounds->m_Sound >= 0 ? m_Map.m_vpSounds[pSounds->m_Sound]->m_aName : "Sounds");
}
if(str_length(aBuf) > 11)
str_format(aBuf, sizeof(aBuf), "%.8s...", aBuf);
str_format(aBuf, sizeof(aBuf), "%.8s", aBuf);
}
float FontSize = 10.0f;
@ -4069,7 +4069,7 @@ bool CEditor::AddImage(const char *pFileName, int StorageType, void *pUser)
TextureLoadFlag = 0;
pImg->m_Texture = pEditor->Graphics()->LoadTextureRaw(ImgInfo.m_Width, ImgInfo.m_Height, ImgInfo.m_Format, ImgInfo.m_pData, CImageInfo::FORMAT_AUTO, TextureLoadFlag, pFileName);
ImgInfo.m_pData = nullptr;
str_copy(pImg->m_aName, aBuf, sizeof(pImg->m_aName));
str_copy(pImg->m_aName, aBuf);
pImg->m_AutoMapper.Load(pImg->m_aName);
pEditor->m_Map.m_vpImages.push_back(pImg);
pEditor->SortImages();
@ -4125,7 +4125,7 @@ bool CEditor::AddSound(const char *pFileName, int StorageType, void *pUser)
pSound->m_SoundID = SoundId;
pSound->m_DataSize = DataSize;
pSound->m_pData = pData;
str_copy(pSound->m_aName, aBuf, sizeof(pSound->m_aName));
str_copy(pSound->m_aName, aBuf);
pEditor->m_Map.m_vpSounds.push_back(pSound);
if(pEditor->m_SelectedSound >= 0 && (size_t)pEditor->m_SelectedSound < pEditor->m_Map.m_vpSounds.size())
@ -4534,7 +4534,7 @@ static int EditorListdirCallback(const CFsFileInfo *pInfo, int IsDir, int Storag
return 0;
CEditor::CFilelistItem Item;
str_copy(Item.m_aFilename, pInfo->m_pName, sizeof(Item.m_aFilename));
str_copy(Item.m_aFilename, pInfo->m_pName);
if(IsDir)
str_format(Item.m_aName, sizeof(Item.m_aName), "%s/", pInfo->m_pName);
else
@ -4889,12 +4889,12 @@ void CEditor::RenderFileDialog()
if(m_vpFilteredFileList[m_FilesSelectedIndex]->m_IsLink)
{
m_pFileDialogPath = m_aFileDialogCurrentLink; // follow the link
str_copy(m_aFileDialogCurrentLink, m_vpFilteredFileList[m_FilesSelectedIndex]->m_aFilename, sizeof(m_aFileDialogCurrentLink));
str_copy(m_aFileDialogCurrentLink, m_vpFilteredFileList[m_FilesSelectedIndex]->m_aFilename);
}
else
{
char aTemp[IO_MAX_PATH_LENGTH];
str_copy(aTemp, m_pFileDialogPath, sizeof(aTemp));
str_copy(aTemp, m_pFileDialogPath);
str_format(m_pFileDialogPath, IO_MAX_PATH_LENGTH, "%s/%s", aTemp, m_vpFilteredFileList[m_FilesSelectedIndex]->m_aFilename);
}
}
@ -5036,8 +5036,8 @@ void CEditor::FilelistPopulate(int StorageType, bool KeepSelection)
if(m_FileDialogStorageType != IStorage::TYPE_SAVE && !str_comp(m_pFileDialogPath, "maps"))
{
CFilelistItem Item;
str_copy(Item.m_aFilename, "downloadedmaps", sizeof(Item.m_aFilename));
str_copy(Item.m_aName, "downloadedmaps/", sizeof(Item.m_aName));
str_copy(Item.m_aFilename, "downloadedmaps");
str_copy(Item.m_aName, "downloadedmaps/");
Item.m_IsDir = true;
Item.m_IsLink = true;
Item.m_StorageType = IStorage::TYPE_SAVE;
@ -5079,7 +5079,7 @@ void CEditor::InvokeFileDialog(int StorageType, int FileType, const char *pTitle
if(pDefaultName)
m_FileDialogFileNameInput.Set(pDefaultName);
if(pBasePath)
str_copy(m_aFileDialogCurrentFolder, pBasePath, sizeof(m_aFileDialogCurrentFolder));
str_copy(m_aFileDialogCurrentFolder, pBasePath);
FilelistPopulate(m_FileDialogStorageType);
@ -5166,7 +5166,7 @@ void CEditor::RenderStatusbar(CUIRect View)
if(ms_pUiGotContext && ms_pUiGotContext == UI()->HotItem())
str_format(aBuf, sizeof(aBuf), "%s Right click for context menu.", m_pTooltip);
else
str_copy(aBuf, m_pTooltip, sizeof(aBuf));
str_copy(aBuf, m_pTooltip);
float FontSize = ScaleFontSize(aBuf, sizeof(aBuf), 10.0f, View.w);
SLabelProperties Props;
@ -6007,7 +6007,7 @@ void CEditor::RenderMenubar(CUIRect MenuBar)
MenuBar.VSplitRight(20.0f, &MenuBar, &Close);
Close.VSplitLeft(5.0f, nullptr, &Close);
MenuBar.VSplitLeft(MenuBar.w * 0.75f, &MenuBar, &Info);
char aBuf[128];
char aBuf[IO_MAX_PATH_LENGTH + 32];
str_format(aBuf, sizeof(aBuf), "File: %s", m_aFileName);
UI()->DoLabel(&MenuBar, aBuf, 10.0f, TEXTALIGN_ML);
@ -6147,10 +6147,10 @@ void CEditor::Render()
// show mentions
if(m_GuiActive && m_Mentions)
{
char aBuf[16];
char aBuf[64];
if(m_Mentions == 1)
{
str_copy(aBuf, Localize("1 new mention"), sizeof(aBuf));
str_copy(aBuf, Localize("1 new mention"));
}
else if(m_Mentions <= 9)
{
@ -6158,7 +6158,7 @@ void CEditor::Render()
}
else
{
str_copy(aBuf, Localize("9+ new mentions"), sizeof(aBuf));
str_copy(aBuf, Localize("9+ new mentions"));
}
TextRender()->TextColor(1.0f, 0.0f, 0.0f, 1.0f);
@ -6245,7 +6245,7 @@ void CEditor::Render()
{
if(!m_PopupEventWasActivated)
{
str_copy(m_aFileSaveName, m_aFileName, sizeof(m_aFileSaveName));
str_copy(m_aFileSaveName, m_aFileName);
CallbackSaveMap(m_aFileSaveName, IStorage::TYPE_SAVE, this);
}
}
@ -6608,7 +6608,7 @@ void CEditorMap::MakeGameGroup(CLayerGroup *pGroup)
{
m_pGameGroup = pGroup;
m_pGameGroup->m_GameGroup = true;
str_copy(m_pGameGroup->m_aName, "Game", sizeof(m_pGameGroup->m_aName));
str_copy(m_pGameGroup->m_aName, "Game");
}
void CEditorMap::Clean()

View file

@ -120,7 +120,7 @@ public:
CLayer()
{
m_Type = LAYERTYPE_INVALID;
str_copy(m_aName, "(invalid)", sizeof(m_aName));
str_copy(m_aName, "(invalid)");
m_Visible = true;
m_Readonly = false;
m_Flags = 0;
@ -130,7 +130,7 @@ public:
CLayer(const CLayer &Other)
{
str_copy(m_aName, Other.m_aName, sizeof(m_aName));
str_copy(m_aName, Other.m_aName);
m_Flags = Other.m_Flags;
m_pEditor = Other.m_pEditor;
m_Type = Other.m_Type;

View file

@ -403,7 +403,7 @@ bool CEditor::Load(const char *pFileName, int StorageType)
bool Result = m_Map.Load(Kernel()->RequestInterface<IStorage>(), pFileName, StorageType);
if(Result)
{
str_copy(m_aFileName, pFileName, 512);
str_copy(m_aFileName, pFileName);
SortImages();
SelectGameLayer();
ResetMenuBackgroundPositions();
@ -445,13 +445,13 @@ bool CEditorMap::Load(class IStorage *pStorage, const char *pFileName, int Stora
continue;
if(pItem->m_Author > -1)
str_copy(m_MapInfo.m_aAuthor, (char *)DataFile.GetData(pItem->m_Author), sizeof(m_MapInfo.m_aAuthor));
str_copy(m_MapInfo.m_aAuthor, (char *)DataFile.GetData(pItem->m_Author));
if(pItem->m_MapVersion > -1)
str_copy(m_MapInfo.m_aVersion, (char *)DataFile.GetData(pItem->m_MapVersion), sizeof(m_MapInfo.m_aVersion));
str_copy(m_MapInfo.m_aVersion, (char *)DataFile.GetData(pItem->m_MapVersion));
if(pItem->m_Credits > -1)
str_copy(m_MapInfo.m_aCredits, (char *)DataFile.GetData(pItem->m_Credits), sizeof(m_MapInfo.m_aCredits));
str_copy(m_MapInfo.m_aCredits, (char *)DataFile.GetData(pItem->m_Credits));
if(pItem->m_License > -1)
str_copy(m_MapInfo.m_aLicense, (char *)DataFile.GetData(pItem->m_License), sizeof(m_MapInfo.m_aLicense));
str_copy(m_MapInfo.m_aLicense, (char *)DataFile.GetData(pItem->m_License));
if(pItem->m_Version != 1 || ItemSize < (int)sizeof(CMapItemInfoSettings))
break;
@ -466,7 +466,7 @@ bool CEditorMap::Load(class IStorage *pStorage, const char *pFileName, int Stora
{
int StrSize = str_length(pNext) + 1;
CSetting Setting;
str_copy(Setting.m_aCommand, pNext, sizeof(Setting.m_aCommand));
str_copy(Setting.m_aCommand, pNext);
m_vSettings.push_back(Setting);
pNext += StrSize;
}
@ -523,7 +523,7 @@ bool CEditorMap::Load(class IStorage *pStorage, const char *pFileName, int Stora
// copy image name
if(pName)
str_copy(pImg->m_aName, pName, 128);
str_copy(pImg->m_aName, pName);
// load auto mapper file
pImg->m_AutoMapper.Load(pImg->m_aName);
@ -572,7 +572,7 @@ bool CEditorMap::Load(class IStorage *pStorage, const char *pFileName, int Stora
// copy image name
if(pName)
str_copy(pSound->m_aName, pName, sizeof(pSound->m_aName));
str_copy(pSound->m_aName, pName);
m_vpSounds.push_back(pSound);

View file

@ -5,7 +5,7 @@
CLayerGame::CLayerGame(int w, int h) :
CLayerTiles(w, h)
{
str_copy(m_aName, "Game", sizeof(m_aName));
str_copy(m_aName, "Game");
m_Game = 1;
}

View file

@ -265,7 +265,7 @@ int CLayerTiles::BrushGrab(CLayerGroup *pBrush, CUIRect Rect)
}
}
pGrabbed->m_TeleNum = m_pEditor->m_TeleNumber;
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName, sizeof(pGrabbed->m_aFileName));
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName);
}
else if(this->m_Speedup)
{
@ -303,7 +303,7 @@ int CLayerTiles::BrushGrab(CLayerGroup *pBrush, CUIRect Rect)
pGrabbed->m_SpeedupForce = m_pEditor->m_SpeedupForce;
pGrabbed->m_SpeedupMaxSpeed = m_pEditor->m_SpeedupMaxSpeed;
pGrabbed->m_SpeedupAngle = m_pEditor->m_SpeedupAngle;
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName, sizeof(pGrabbed->m_aFileName));
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName);
}
else if(this->m_Switch)
{
@ -339,7 +339,7 @@ int CLayerTiles::BrushGrab(CLayerGroup *pBrush, CUIRect Rect)
}
pGrabbed->m_SwitchNumber = m_pEditor->m_SwitchNum;
pGrabbed->m_SwitchDelay = m_pEditor->m_SwitchDelay;
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName, sizeof(pGrabbed->m_aFileName));
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName);
}
else if(this->m_Tune)
@ -374,7 +374,7 @@ int CLayerTiles::BrushGrab(CLayerGroup *pBrush, CUIRect Rect)
}
}
pGrabbed->m_TuningNumber = m_pEditor->m_TuningNum;
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName, sizeof(pGrabbed->m_aFileName));
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName);
}
else if(this->m_Front)
{
@ -395,7 +395,7 @@ int CLayerTiles::BrushGrab(CLayerGroup *pBrush, CUIRect Rect)
for(int y = 0; y < r.h; y++)
for(int x = 0; x < r.w; x++)
pGrabbed->m_pTiles[y * pGrabbed->m_Width + x] = GetTile(r.x + x, r.y + y);
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName, sizeof(pGrabbed->m_aFileName));
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName);
}
else
{
@ -416,7 +416,7 @@ int CLayerTiles::BrushGrab(CLayerGroup *pBrush, CUIRect Rect)
for(int y = 0; y < r.h; y++)
for(int x = 0; x < r.w; x++)
pGrabbed->m_pTiles[y * pGrabbed->m_Width + x] = GetTile(r.x + x, r.y + y);
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName, sizeof(pGrabbed->m_aFileName));
str_copy(pGrabbed->m_aFileName, m_pEditor->m_aFileName);
}
return 1;
@ -1102,7 +1102,7 @@ void CLayerTiles::ModifyEnvelopeIndex(INDEX_MODIFY_FUNC Func)
CLayerTele::CLayerTele(int w, int h) :
CLayerTiles(w, h)
{
str_copy(m_aName, "Tele", sizeof(m_aName));
str_copy(m_aName, "Tele");
m_Tele = 1;
m_pTeleTile = new CTeleTile[w * h];
@ -1348,7 +1348,7 @@ bool CLayerTele::ContainsElementWithId(int Id)
CLayerSpeedup::CLayerSpeedup(int w, int h) :
CLayerTiles(w, h)
{
str_copy(m_aName, "Speedup", sizeof(m_aName));
str_copy(m_aName, "Speedup");
m_Speedup = 1;
m_pSpeedupTile = new CSpeedupTile[w * h];
@ -1591,7 +1591,7 @@ void CLayerSpeedup::FillSelection(bool Empty, CLayer *pBrush, CUIRect Rect)
CLayerFront::CLayerFront(int w, int h) :
CLayerTiles(w, h)
{
str_copy(m_aName, "Front", sizeof(m_aName));
str_copy(m_aName, "Front");
m_Front = 1;
}
@ -1637,7 +1637,7 @@ void CLayerFront::Resize(int NewW, int NewH)
CLayerSwitch::CLayerSwitch(int w, int h) :
CLayerTiles(w, h)
{
str_copy(m_aName, "Switch", sizeof(m_aName));
str_copy(m_aName, "Switch");
m_Switch = 1;
m_pSwitchTile = new CSwitchTile[w * h];
@ -1908,7 +1908,7 @@ bool CLayerSwitch::ContainsElementWithId(int Id)
CLayerTune::CLayerTune(int w, int h) :
CLayerTiles(w, h)
{
str_copy(m_aName, "Tune", sizeof(m_aName));
str_copy(m_aName, "Tune");
m_Tune = 1;
m_pTuneTile = new CTuneTile[w * h];

View file

@ -90,7 +90,7 @@ CUI::EPopupMenuFunctionResult CEditor::PopupMenuFile(void *pContext, CUIRect Vie
{
if(pEditor->m_aFileName[0] && pEditor->m_ValidSaveFilename)
{
str_copy(pEditor->m_aFileSaveName, pEditor->m_aFileName, sizeof(pEditor->m_aFileSaveName));
str_copy(pEditor->m_aFileSaveName, pEditor->m_aFileName);
pEditor->m_PopupEventType = POPEVENT_SAVE;
pEditor->m_PopupEventActivated = true;
}