Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Commit

Permalink
Replace ZeroMemory() with SecureZeroMemory(). The MSDN article for Ze…
Browse files Browse the repository at this point in the history 
…roMemory says: "To avoid any undesired effects of optimizing compilers, use the SecureZeroMemory function."
  • Loading branch information
@XhmikosR
XhmikosR committed Dec 27, 2012
1 parent d5a5182 commit 0533530
Show file tree
Hide file tree
Showing 36 changed files with 96 additions and 96 deletions.
2 changes: 1 addition & 1 deletion src/DSUtil/DSUtil.h
View file
Expand Up @@ -265,7 +265,7 @@ class CFilterInfo : public FILTER_INFO

template <typename T> __inline void INITDDSTRUCT(T& dd)
{
ZeroMemory(&dd, sizeof(dd));
SecureZeroMemory(&dd, sizeof(dd));
dd.dwSize = sizeof(dd);
}

Expand Down
4 changes: 2 additions & 2 deletions src/DSUtil/NullRenderers.cpp
View file
Expand Up @@ -154,11 +154,11 @@ void CNullVideoRendererInputPin::CreateSurface()
m_hWnd = NULL; // TODO : put true window

D3DDISPLAYMODE d3ddm;
ZeroMemory(&d3ddm, sizeof(d3ddm));
SecureZeroMemory(&d3ddm, sizeof(d3ddm));
m_pD3D->GetAdapterDisplayMode(D3DADAPTER_DEFAULT, &d3ddm);

D3DPRESENT_PARAMETERS pp;
ZeroMemory(&pp, sizeof(pp));
SecureZeroMemory(&pp, sizeof(pp));

pp.Windowed = TRUE;
pp.hDeviceWindow = m_hWnd;
Expand Down
10 changes: 5 additions & 5 deletions src/SubPic/DX9SubPic.cpp
View file
Expand Up @@ -33,7 +33,7 @@ CDX9SubPic::CDX9SubPic(IDirect3DSurface9* pSurface, CDX9SubPicAllocator* pAlloca
, m_bExternalRenderer(bExternalRenderer)
{
D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (SUCCEEDED(m_pSurface->GetDesc(&d3dsd))) {
m_maxsize.SetSize(d3dsd.Width, d3dsd.Height);
m_rcDirty.SetRect(0, 0, d3dsd.Width, d3dsd.Height);
Expand Down Expand Up @@ -75,7 +75,7 @@ STDMETHODIMP_(void*) CDX9SubPic::GetObject()
STDMETHODIMP CDX9SubPic::GetDesc(SubPicDesc& spd)
{
D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(m_pSurface->GetDesc(&d3dsd))) {
return E_FAIL;
}
Expand Down Expand Up @@ -176,13 +176,13 @@ STDMETHODIMP CDX9SubPic::ClearDirtyRect(DWORD color)
STDMETHODIMP CDX9SubPic::Lock(SubPicDesc& spd)
{
D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(m_pSurface->GetDesc(&d3dsd))) {
return E_FAIL;
}

D3DLOCKED_RECT LockedRect;
ZeroMemory(&LockedRect, sizeof(LockedRect));
SecureZeroMemory(&LockedRect, sizeof(LockedRect));
if (FAILED(m_pSurface->LockRect(&LockedRect, NULL, D3DLOCK_NO_DIRTY_UPDATE | D3DLOCK_NOSYSLOCK))) {
return E_FAIL;
}
Expand Down Expand Up @@ -246,7 +246,7 @@ STDMETHODIMP CDX9SubPic::AlphaBlt(RECT* pSrc, RECT* pDst, SubPicDesc* pTarget)

do {
D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(pTexture->GetLevelDesc(0, &d3dsd)) /*|| d3dsd.Type != D3DRTYPE_TEXTURE*/) {
break;
}
Expand Down
2 changes: 1 addition & 1 deletion src/filters/muxer/WavDest/WavDest.cpp
View file
Expand Up @@ -297,7 +297,7 @@ HRESULT CWavDestFilter::StopStreaming()
pRiffWave->fccListType = FCC('WAVE');

LARGE_INTEGER li;
ZeroMemory(&li, sizeof(li));
SecureZeroMemory(&li, sizeof(li));

hr = pStream->Seek(li, STREAM_SEEK_SET, 0);
if (SUCCEEDED(hr)) {
Expand Down
4 changes: 2 additions & 2 deletions src/filters/renderer/MpcAudioRenderer/MpcAudioRenderer.cpp
View file
Expand Up @@ -665,8 +665,8 @@ HRESULT CMpcAudioRenderer::CreateDSBuffer()
DSBCAPS bufferCaps;
DWORD dwDSBufSize = m_pWaveFileFormat->nAvgBytesPerSec * 4;

ZeroMemory(&bufferCaps, sizeof(bufferCaps));
ZeroMemory(&dsbd, sizeof(DSBUFFERDESC));
SecureZeroMemory(&bufferCaps, sizeof(bufferCaps));
SecureZeroMemory(&dsbd, sizeof(DSBUFFERDESC));

dsbd.dwSize = sizeof(DSBUFFERDESC);
dsbd.dwFlags = DSBCAPS_PRIMARYBUFFER;
Expand Down
2 changes: 1 addition & 1 deletion src/filters/renderer/VideoRenderers/D3DFont.cpp
View file
Expand Up @@ -278,7 +278,7 @@ HRESULT CD3DFont::InitDeviceObjects(LPDIRECT3DDEVICE9 pd3dDevice)
// Prepare to create a bitmap
DWORD* pBitmapBits;
BITMAPINFO bmi;
ZeroMemory(&bmi.bmiHeader, sizeof(BITMAPINFOHEADER));
SecureZeroMemory(&bmi.bmiHeader, sizeof(BITMAPINFOHEADER));
bmi.bmiHeader.biSize = sizeof(BITMAPINFOHEADER);
bmi.bmiHeader.biWidth = (int)m_dwTexWidth;
bmi.bmiHeader.biHeight = -(int)m_dwTexHeight;
Expand Down
22 changes: 11 additions & 11 deletions src/filters/renderer/VideoRenderers/DX9AllocatorPresenter.cpp
View file
Expand Up @@ -128,13 +128,13 @@ CDX9AllocatorPresenter::CDX9AllocatorPresenter(HWND hWnd, bool bFullscreen, HRES
m_DetectedFrameTime = 0.0;
m_DetectedFrameTimeStdDev = 0.0;
m_DetectedLock = false;
ZeroMemory(m_DetectedFrameTimeHistory, sizeof(m_DetectedFrameTimeHistory));
ZeroMemory(m_DetectedFrameTimeHistoryHistory, sizeof(m_DetectedFrameTimeHistoryHistory));
SecureZeroMemory(m_DetectedFrameTimeHistory, sizeof(m_DetectedFrameTimeHistory));
SecureZeroMemory(m_DetectedFrameTimeHistoryHistory, sizeof(m_DetectedFrameTimeHistoryHistory));
m_DetectedFrameTimePos = 0;
ZeroMemory(&m_VMR9AlphaBitmap, sizeof(m_VMR9AlphaBitmap));
SecureZeroMemory(&m_VMR9AlphaBitmap, sizeof(m_VMR9AlphaBitmap));

ZeroMemory(m_ldDetectedRefreshRateList, sizeof(m_ldDetectedRefreshRateList));
ZeroMemory(m_ldDetectedScanlineRateList, sizeof(m_ldDetectedScanlineRateList));
SecureZeroMemory(m_ldDetectedRefreshRateList, sizeof(m_ldDetectedRefreshRateList));
SecureZeroMemory(m_ldDetectedScanlineRateList, sizeof(m_ldDetectedScanlineRateList));
m_DetectedRefreshRatePos = 0;
m_DetectedRefreshTimePrim = 0;
m_DetectedScanlineTime = 0;
Expand Down Expand Up @@ -697,8 +697,8 @@ HRESULT CDX9AllocatorPresenter::CreateDevice(CString& _Error)
m_ModeratedTimeSpeed = 1.0;
m_ModeratedTimeSpeedDiff = 0.0;
m_ModeratedTimeSpeedPrim = 0;
ZeroMemory(m_TimeChangeHistory, sizeof(m_TimeChangeHistory));
ZeroMemory(m_ClockChangeHistory, sizeof(m_ClockChangeHistory));
SecureZeroMemory(m_TimeChangeHistory, sizeof(m_TimeChangeHistory));
SecureZeroMemory(m_ClockChangeHistory, sizeof(m_ClockChangeHistory));
m_ClockTimeChangeHistoryPos = 0;

m_pD3DDev = NULL;
Expand Down Expand Up @@ -759,7 +759,7 @@ HRESULT CDX9AllocatorPresenter::CreateDevice(CString& _Error)
#endif

D3DPRESENT_PARAMETERS pp;
ZeroMemory(&pp, sizeof(pp));
SecureZeroMemory(&pp, sizeof(pp));

BOOL bCompositionEnabled = false;
if (m_pDwmIsCompositionEnabled) {
Expand Down Expand Up @@ -798,10 +798,10 @@ HRESULT CDX9AllocatorPresenter::CreateDevice(CString& _Error)
}

D3DDISPLAYMODEEX DisplayMode;
ZeroMemory(&DisplayMode, sizeof(DisplayMode));
SecureZeroMemory(&DisplayMode, sizeof(DisplayMode));
DisplayMode.Size = sizeof(DisplayMode);
D3DDISPLAYMODE d3ddm;
ZeroMemory(&d3ddm, sizeof(d3ddm));
SecureZeroMemory(&d3ddm, sizeof(d3ddm));

if (m_bIsFullscreen) {
if (m_bHighColorResolution) {
Expand Down Expand Up @@ -933,7 +933,7 @@ HRESULT CDX9AllocatorPresenter::CreateDevice(CString& _Error)
}

// Get the device caps
ZeroMemory(&m_Caps, sizeof(m_Caps));
SecureZeroMemory(&m_Caps, sizeof(m_Caps));
m_pD3DDev->GetDeviceCaps(&m_Caps);

// Initialize the rendering engine
Expand Down
4 changes: 2 additions & 2 deletions src/filters/renderer/VideoRenderers/DX9RenderingEngine.cpp
View file
Expand Up @@ -1481,7 +1481,7 @@ HRESULT CDX9RenderingEngine::TextureCopy(IDirect3DTexture9* pTexture)
bool CDX9RenderingEngine::ClipToSurface(IDirect3DSurface9* pSurface, CRect& s, CRect& d)
{
D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(pSurface->GetDesc(&d3dsd))) {
return false;
}
Expand Down Expand Up @@ -1572,7 +1572,7 @@ HRESULT CDX9RenderingEngine::AlphaBlt(RECT* pSrc, RECT* pDst, IDirect3DTexture9*
HRESULT hr;

D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(pTexture->GetLevelDesc(0, &d3dsd)) /*|| d3dsd.Type != D3DRTYPE_TEXTURE*/) {
return E_FAIL;
}
Expand Down
18 changes: 9 additions & 9 deletions src/filters/renderer/VideoRenderers/EVRAllocatorPresenter.cpp
View file
Expand Up @@ -178,7 +178,7 @@ CEVRAllocatorPresenter::CEVRAllocatorPresenter(HWND hWnd, bool bFullscreen, HRES
m_LastScheduledUncorrectedSampleTime = -1;
m_MaxSampleDuration = 0;
m_LastSampleOffset = 0;
ZeroMemory(m_VSyncOffsetHistory, sizeof(m_VSyncOffsetHistory));
SecureZeroMemory(m_VSyncOffsetHistory, sizeof(m_VSyncOffsetHistory));
m_VSyncOffsetHistoryPos = 0;
m_bLastSampleOffsetValid = false;
}
Expand Down Expand Up @@ -1243,7 +1243,7 @@ STDMETHODIMP CEVRAllocatorPresenter::GetIdealVideoSize(SIZE* pszMin, SIZE* pszMa
if (pszMax) {
D3DDISPLAYMODE d3ddm;

ZeroMemory(&d3ddm, sizeof(d3ddm));
SecureZeroMemory(&d3ddm, sizeof(d3ddm));
if (SUCCEEDED(m_pD3D->GetAdapterDisplayMode(GetAdapter(m_pD3D), &d3ddm))) {
pszMax->cx = d3ddm.Width;
pszMax->cy = d3ddm.Height;
Expand Down Expand Up @@ -1670,8 +1670,8 @@ LONGLONG CEVRAllocatorPresenter::GetClockTime(LONGLONG PerformanceCounter)
if (bReset) {
m_ModeratedTimeSpeed = 1.0;
m_ModeratedTimeSpeedPrim = 0.0;
ZeroMemory(m_TimeChangeHistory, sizeof(m_TimeChangeHistory));
ZeroMemory(m_ClockChangeHistory, sizeof(m_ClockChangeHistory));
SecureZeroMemory(m_TimeChangeHistory, sizeof(m_TimeChangeHistory));
SecureZeroMemory(m_ClockChangeHistory, sizeof(m_ClockChangeHistory));
m_ClockTimeChangeHistoryPos = 0;
}
if (TimeChangeM) {
Expand Down Expand Up @@ -1891,13 +1891,13 @@ STDMETHODIMP_(bool) CEVRAllocatorPresenter::DisplayChange()
m_DetectedFrameTime = 0.0;
m_DetectedFrameTimeStdDev = 0.0;
m_DetectedLock = false;
ZeroMemory(m_DetectedFrameTimeHistory, sizeof(m_DetectedFrameTimeHistory));
ZeroMemory(m_DetectedFrameTimeHistoryHistory, sizeof(m_DetectedFrameTimeHistoryHistory));
SecureZeroMemory(m_DetectedFrameTimeHistory, sizeof(m_DetectedFrameTimeHistory));
SecureZeroMemory(m_DetectedFrameTimeHistoryHistory, sizeof(m_DetectedFrameTimeHistoryHistory));
m_DetectedFrameTimePos = 0;
ZeroMemory(&m_VMR9AlphaBitmap, sizeof(m_VMR9AlphaBitmap));
SecureZeroMemory(&m_VMR9AlphaBitmap, sizeof(m_VMR9AlphaBitmap));

ZeroMemory(m_ldDetectedRefreshRateList, sizeof(m_ldDetectedRefreshRateList));
ZeroMemory(m_ldDetectedScanlineRateList, sizeof(m_ldDetectedScanlineRateList));
SecureZeroMemory(m_ldDetectedRefreshRateList, sizeof(m_ldDetectedRefreshRateList));
SecureZeroMemory(m_ldDetectedScanlineRateList, sizeof(m_ldDetectedScanlineRateList));
m_DetectedRefreshRatePos = 0;
m_DetectedRefreshTimePrim = 0;
m_DetectedScanlineTime = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/filters/renderer/VideoRenderers/QT9AllocatorPresenter.cpp
View file
Expand Up @@ -86,7 +86,7 @@ STDMETHODIMP CQT9AllocatorPresenter::DoBlt(const BITMAP& bm)
bool fOk = false;

D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(m_pVideoSurfaceOff->GetDesc(&d3dsd))) {
return E_FAIL;
}
Expand Down
2 changes: 1 addition & 1 deletion src/filters/renderer/VideoRenderers/RM9AllocatorPresenter.cpp
View file
Expand Up @@ -98,7 +98,7 @@ STDMETHODIMP CRM9AllocatorPresenter::Blt(UCHAR* pImageData, RMABitmapInfoHeader*
}

D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(m_pVideoSurfaceOff->GetDesc(&d3dsd))) {
return E_FAIL;
}
Expand Down
24 changes: 12 additions & 12 deletions src/filters/renderer/VideoRenderers/SyncRenderer.cpp
View file
Expand Up @@ -149,7 +149,7 @@ CBaseAP::CBaseAP(HWND hWnd, bool bFullscreen, HRESULT& hr, CString& _Error):
m_pD3D = m_pD3DEx;
}

ZeroMemory(&m_VMR9AlphaBitmap, sizeof(m_VMR9AlphaBitmap));
SecureZeroMemory(&m_VMR9AlphaBitmap, sizeof(m_VMR9AlphaBitmap));

CRenderersSettings& s = GetRenderersSettings();
if (s.m_AdvRendSets.iVMRDisableDesktopComposition) {
Expand Down Expand Up @@ -412,7 +412,7 @@ HRESULT CBaseAP::CreateDXDevice(CString& _Error)
}

D3DDISPLAYMODE d3ddm;
ZeroMemory(&d3ddm, sizeof(d3ddm));
SecureZeroMemory(&d3ddm, sizeof(d3ddm));
m_CurrentAdapter = GetAdapter(m_pD3D, m_hWnd);
if (FAILED(m_pD3D->GetAdapterDisplayMode(m_CurrentAdapter, &d3ddm))) {
_Error += L"Can not retrieve display mode data\n";
Expand All @@ -436,7 +436,7 @@ HRESULT CBaseAP::CreateDXDevice(CString& _Error)
}
m_bCompositionEnabled = bCompositionEnabled != 0;

ZeroMemory(&pp, sizeof(pp));
SecureZeroMemory(&pp, sizeof(pp));
if (m_bIsFullscreen) { // Exclusive mode fullscreen
pp.Windowed = FALSE;
pp.BackBufferWidth = d3ddm.Width;
Expand All @@ -463,7 +463,7 @@ HRESULT CBaseAP::CreateDXDevice(CString& _Error)

if (m_pD3DEx) {
D3DDISPLAYMODEEX DisplayMode;
ZeroMemory(&DisplayMode, sizeof(DisplayMode));
SecureZeroMemory(&DisplayMode, sizeof(DisplayMode));
DisplayMode.Size = sizeof(DisplayMode);
m_pD3DEx->GetAdapterDisplayModeEx(m_CurrentAdapter, &DisplayMode, NULL);

Expand Down Expand Up @@ -672,7 +672,7 @@ HRESULT CBaseAP::ResetDXDevice(CString& _Error)

// Disconnect all pins to release video memory resources
if (m_pD3DDev) {
ZeroMemory(&filterInfo, sizeof(filterInfo));
SecureZeroMemory(&filterInfo, sizeof(filterInfo));
m_pOuterEVR->QueryFilterInfo(&filterInfo); // This addref's the pGraph member
if (SUCCEEDED(m_pOuterEVR->EnumPins(&rendererInputEnum))) {
CComPtr<IPin> input;
Expand Down Expand Up @@ -723,7 +723,7 @@ HRESULT CBaseAP::ResetDXDevice(CString& _Error)
}

D3DDISPLAYMODE d3ddm;
ZeroMemory(&d3ddm, sizeof(d3ddm));
SecureZeroMemory(&d3ddm, sizeof(d3ddm));
if (FAILED(m_pD3D->GetAdapterDisplayMode(GetAdapter(m_pD3D, m_hWnd), &d3ddm))) {
_Error += L"Can not retrieve display mode data\n";
return E_UNEXPECTED;
Expand All @@ -735,7 +735,7 @@ HRESULT CBaseAP::ResetDXDevice(CString& _Error)
m_pGenlock->SetDisplayResolution(d3ddm.Width, d3ddm.Height);

D3DPRESENT_PARAMETERS pp;
ZeroMemory(&pp, sizeof(pp));
SecureZeroMemory(&pp, sizeof(pp));

BOOL bCompositionEnabled = false;
if (m_pDwmIsCompositionEnabled) {
Expand All @@ -758,7 +758,7 @@ HRESULT CBaseAP::ResetDXDevice(CString& _Error)
}

D3DDISPLAYMODEEX DisplayMode;
ZeroMemory(&DisplayMode, sizeof(DisplayMode));
SecureZeroMemory(&DisplayMode, sizeof(DisplayMode));
DisplayMode.Size = sizeof(DisplayMode);
if (m_pD3DDevEx) {
m_pD3DEx->GetAdapterDisplayModeEx(GetAdapter(m_pD3DEx, m_hWnd), &DisplayMode, NULL);
Expand Down Expand Up @@ -991,7 +991,7 @@ STDMETHODIMP CBaseAP::CreateRenderer(IUnknown** ppRenderer)
bool CBaseAP::ClipToSurface(IDirect3DSurface9* pSurface, CRect& s, CRect& d)
{
D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(pSurface->GetDesc(&d3dsd))) {
return false;
}
Expand Down Expand Up @@ -1368,7 +1368,7 @@ HRESULT CBaseAP::AlphaBlt(RECT* pSrc, RECT* pDst, IDirect3DTexture9* pTexture)

do {
D3DSURFACE_DESC d3dsd;
ZeroMemory(&d3dsd, sizeof(d3dsd));
SecureZeroMemory(&d3dsd, sizeof(d3dsd));
if (FAILED(pTexture->GetLevelDesc(0, &d3dsd)) /*|| d3dsd.Type != D3DRTYPE_TEXTURE*/) {
break;
}
Expand Down Expand Up @@ -3272,7 +3272,7 @@ STDMETHODIMP CSyncAP::GetIdealVideoSize(SIZE* pszMin, SIZE* pszMax)
if (pszMax) {
D3DDISPLAYMODE d3ddm;

ZeroMemory(&d3ddm, sizeof(d3ddm));
SecureZeroMemory(&d3ddm, sizeof(d3ddm));
if (SUCCEEDED(m_pD3D->GetAdapterDisplayMode(GetAdapter(m_pD3D, m_hWnd), &d3ddm))) {
pszMax->cx = d3ddm.Width;
pszMax->cy = d3ddm.Height;
Expand Down Expand Up @@ -3897,7 +3897,7 @@ HRESULT CSyncAP::BeginStreaming()

CComPtr<IBaseFilter> pEVR;
FILTER_INFO filterInfo;
ZeroMemory(&filterInfo, sizeof(filterInfo));
SecureZeroMemory(&filterInfo, sizeof(filterInfo));
m_pOuterEVR->QueryInterface(__uuidof(IBaseFilter), (void**)&pEVR);
pEVR->QueryFilterInfo(&filterInfo); // This addref's the pGraph member

Expand Down
2 changes: 1 addition & 1 deletion src/filters/transform/MPCVideoDec/MPCVideoDecFilter.cpp
View file
Expand Up @@ -2200,7 +2200,7 @@ HRESULT CMPCVideoDecFilter::ConfigureDXVA2(IPin* pPin)
GUID guidDecoder = GUID_NULL;

DXVA2_ConfigPictureDecode config;
ZeroMemory(&config, sizeof(config));
SecureZeroMemory(&config, sizeof(config));

CComPtr<IMFGetService> pGetService;
CComPtr<IDirect3DDeviceManager9> pDeviceManager;
Expand Down
2 changes: 1 addition & 1 deletion src/filters/transform/MPCVideoDec/VideoDecDXVAAllocator.cpp
View file
Expand Up @@ -129,7 +129,7 @@ HRESULT CVideoDecDXVAAllocator::Alloc()
if (m_ppRTSurfaceArray == NULL) {
hr = E_OUTOFMEMORY;
} else {
ZeroMemory(m_ppRTSurfaceArray, sizeof(IDirect3DSurface9*) * m_lCount);
SecureZeroMemory(m_ppRTSurfaceArray, sizeof(IDirect3DSurface9*) * m_lCount);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/filters/transform/VSFilter/Systray.cpp
View file
Expand Up @@ -106,7 +106,7 @@ void CSystrayWindow::OnClose()
void CSystrayWindow::OnDestroy()
{
NOTIFYICONDATA tnid;
ZeroMemory(&tnid, sizeof(NOTIFYICONDATA));
SecureZeroMemory(&tnid, sizeof(NOTIFYICONDATA));
tnid.cbSize = sizeof(NOTIFYICONDATA);
tnid.hWnd = m_hWnd;
tnid.uID = IDI_ICON1;
Expand Down Expand Up @@ -156,7 +156,7 @@ LRESULT CSystrayWindow::OnTaskBarRestart(WPARAM, LPARAM)

if (m_tbid->fShowIcon) {
NOTIFYICONDATA tnid;
ZeroMemory(&tnid, sizeof(NOTIFYICONDATA));
SecureZeroMemory(&tnid, sizeof(NOTIFYICONDATA));
tnid.cbSize = sizeof(NOTIFYICONDATA);
tnid.hWnd = m_hWnd;
tnid.uID = IDI_ICON1;
Expand Down
2 changes: 1 addition & 1 deletion src/mpc-hc/LcdSupport.cpp
View file
Expand Up @@ -569,7 +569,7 @@ CMPC_Lcd::CMPC_Lcd()
hLCD_UpdateThread = NULL;

// lcd init
ZeroMemory(&m_ConnCtx, sizeof(m_ConnCtx));
SecureZeroMemory(&m_ConnCtx, sizeof(m_ConnCtx));

m_ConnCtx.appFriendlyName = _T(LCD_APP_NAME);
m_ConnCtx.dwAppletCapabilitiesSupported = LGLCD_APPLET_CAP_BW | LGLCD_APPLET_CAP_QVGA;
Expand Down

21 comments on commit 0533530

@Ede123
Copy link

@Ede123 Ede123 commented on 0533530 Dec 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems as if "undesired effects of optimizing compilers" were mainly security issues when clearing e.g. a password cache with ZeroMemory(), since the call could be delayed or omitted completely by an optimizing compiler were appropriate (see MSDN article for SecureMemoryCopy() - http://msdn.microsoft.com/en-us/library/windows/desktop/aa366877.aspx).

From what I understand functionality isn't affected at all when using a simple ZeroMemory() instead of SecureMemoryCopy(), though the code could be optimized in the first case to run faster. ZeroMemory() is therefore probably still favorable.

@v0lt
Copy link
Contributor

@v0lt v0lt commented on 0533530 Dec 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You misunderstand optimization for ZeroMemory.
In this case, you should not use SecureZeroMemory.

@v0lt
Copy link
Contributor

@v0lt v0lt commented on 0533530 Dec 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"were mainly security issues when clearing e.g. a password cache with ZeroMemory(), since the call could be delayed or omitted completely"

SecureZeroMemory need for passwords and other private data. There is no reason to use it to clean the newly declared variables.

@XhmikosR
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not hurt in either way.

@Ede123
Copy link

@Ede123 Ede123 commented on 0533530 Dec 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your decision...

I'd prefer the potentially faster ZeroMemory() as long as I don't have any real world performance measurements at hand (or did you actually do some benchmarks?).

@XhmikosR
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care about it so no.

@Ede123
Copy link

@Ede123 Ede123 commented on 0533530 Dec 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI:

"The RtlSecureZeroMemory function is written using "volatile". This is a huge hammer and tells the complier "hands off this code, you may think you know what it does, but you don't". Many years ago when Microsoft was doing internal security audits they found that the optimizing compiler was removing memset() operations that were required for security. This page http://msdn.microsoft.com/en-us/library/aa366877(VS.85).aspx gives a hand wavy example of what I’m referring to. In these cases the intent of the programmer was to write over a block of memory to hide its contents from snooping, however the compiler could detect that the memory wasn’t read by the program after that (or something similar that made the memset appear removable) and it removed the memset. The Windows team decided to provide an easy API for developers to use that guaranteed that the memory would be written no matter what. Thus, the SecureZeroMemory API was born. This function is not intended to be used for performance critical code; it is intended to be used for security critical code. Its usage of volatile has the effect of turning off many of the optimizations that would fire on normal code. Compare the output of the compiler when you replace the call to "SecureZeroMemory" with a call to the less security zealous "ZeroMemory" function."

(From http://connect.microsoft.com/VisualStudio/feedback/details/686811/weird-suboptimal-code-emitted-for-securezeromemory)

So it's definitely the wrong function for variable initialization...

@Underground78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure either that the SecureZeroMemory is a good idea for our use case. I think it's being overzealous knowing the potentially bad side effects.

@XhmikosR
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine and it should be used when we need to use ZeroMemory. ZeroMemory can be optimized and thus ignored, and even though we don't handle sensitive data I see no reason to use it when we know that this can happen. Otherwise just don't use it at all.

@Underground78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we use ZeroMemory only for variable initializations, I wouldn't call that security critical while performance can matter.

But anyway probably it doesn't matter much so if you really think SecureZeroMemory is important.

@Nevcairiel
Copy link
Contributor

@Nevcairiel Nevcairiel commented on 0533530 Dec 28, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CrossVR
Copy link
Contributor

@CrossVR CrossVR commented on 0533530 Jan 6, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XhmikosR I appreciate your efforts, but in this case you've misunderstood the MSDN article. If there are no objections I'll revert this commit.

@XhmikosR
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are. I see no reason to use ZeroMemory instead of SecureZeroMemory.

@CrossVR
Copy link
Contributor

@CrossVR CrossVR commented on 0533530 Jan 6, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only force the compiler to ignore optimizations when it is really necessary, or else we might make it ignore optimizations which are actually useful.

@KindDragon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there is also no reason to use SecureZeroMemory instead of ZeroMemory

@XhmikosR
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you have numbers that prove that SecureZeroMemory is faulty or breaks something, it should be preferred. That's all I'm saying and I'm still against reverting it unless I see some proofs.

@CrossVR
Copy link
Contributor

@CrossVR CrossVR commented on 0533530 Jan 6, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you prove that ZeroMemory is faulty or breaks something in our code? I see no reason to prefer SecureZeroMemory here.

We know very well now where SecureZeroMemory should be used and that is just not the case here. The reasons you are giving to use SecureZeroMemory are no longer consistent with the MSDN article.

Also it has a performance penalty: http://connect.microsoft.com/VisualStudio/feedback/details/686811/weird-suboptimal-code-emitted-for-securezeromemory.

Again a Microsoft employee comments there that SecureZeroMemory has been made with "security above all else". We don't need security, we need performance.

@XhmikosR
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I can't prove that, the same way you don't prove the opposite.

For me this discussion is over; unless I see facts that it breaks something or that there is a so important performance penalty, SecureZeroMemory will stay.

@Ede123
Copy link

@Ede123 Ede123 commented on 0533530 Jan 6, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never seen anybody having such a hard time admitting a mistake, especially if it's a mere trifle like in this case.

XhimikosR, your sole argument in this discussion is basically that you don't see a reason to prefer one over the other and therefore you're not going to revert this commit. At the same time every other person that commented has clear objections against using SecureZeroMemory. The conclusion should be obvious...

Regarding performance: From what we now (MSDN) SecureZeroMemory is at best as fast as ZeroMemory, but it's possibly and likely slower. That's basically already enough of a proof to avoid SecureZeroMemory if possible.
Besides that ZeroMemory is the default way to do it. If you want to use SecureZeroMemory (for no obvious reason) it's rather your turn to prove it's not slower!

@XhmikosR
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ede123: no need for lot of words especially from people who are outside of the team and cannot follow our internal discussions.

@CrossVR
Copy link
Contributor

@CrossVR CrossVR commented on 0533530 Jan 6, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ede123 The commit has already been reverted, pressing on this any further won't help.

@XhmikosR Please try not to take the discussion personally, we're all trying to make sure the code is as best as it can be just like you. There is no need to get defensive about your changes, just look at the facts as they stand.

Please sign in to comment.