Notepad++ / Patches / #662 (reserved) Major patch, now compiles cleanly without _CRT_SECURE_NO

The list of changes that I'd been preparing - formatting slightly mutilated by sourceforge:

Codebase-wide changes:
• I replaced the generic_string overload of PathAppend with DynamicPathAppend - which behaves predictably!
• I wrapped dangerous functions with strict_gs_check
• Eliminated lstrcat, lstrcpy, and lstrcpyn
• Code now compiles cleanly without _CRT_SECURE_NO_WARNINGS (with one or two exceptions)
• Added _CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES, which really is awesome!
• Added a new file: "TypesafeMemset.h", which does some additional checks on compile-time-sized objects, to make sure we're not doing anything stupid
○ The main function in this header, templated_helpers::memset_zero_struct, uses a bit of template magic to figure out the size of the object
§ This actually picked up a bug! NMWINDLG::NMWINDLG and NMWINMGR are not POD, and thus calling memset on it produces undefined behavior!
• Changed MainFileManager to NPP_MAIN_FILE_MANAGER, to be more consistent with the style
--->Note to self: what about _CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES_COUNT?
TODO:
• SQLITE_MEMDEBUG
• SQLITE_DEBUG
• SQLITE_ENABLE_API_ARMOR

FileDialog.cpp:
• Found a bug accidentally, with application verifier in addExt - we don't check for CB_ERR (-1) in several places
• Swapped out direct usage of OPENFILENAMENPP struct - which wasn't properly aligned - with classic OPENFILENAME struct, and fixed alignment for OPENFILENAMENPP
• I rewrote FileDialog::setExtsFilter - to fix a nasty set of buffer overruns - but it now uses generic_string instead of a buffer
dpiManager.h:
• Added const
• Updated SAL 1.0 annotations to 2.0
lesDlgs.cpp:
• Moved funcs from header to implementation file
○ ValueDlg::ValueDlg
○ ValueDlg::setNBNumber
○ ValueDlg::destroy
○ ButtonDlg::ButtonDlg
○ ButtonDlg::init
○ ButtonDlg::destroy
○ ButtonDlg::setButtonStatus
○ ButtonDlg::getButtonStatus
○ ButtonDlg::display
lesDlgs.h:
• Class ButtonDlg is now marked final
localization.cpp:
• Access to mainshortcuts was suspicious, so I changed operator[] to .at( )
• Access to scinshortcuts was suspicious, so I changed operator[] to .at( )
• constified parameters for NativeLangSpeaker::searchDlgNode
• constified & annotated parameters and variables for NativeLangSpeaker::changeDlgLangWriteToPtr
• Added a templated version of NativeLangSpeaker::changeDlgLang that writes to an array that's passed by reference
• Swapped generic_sprintf for stprintf_s
Common.cpp:
• Modified getFileContent to accept _In_z PCTSTR instead of const TCHAR
• Swapped generic_fopen for tfopen_s
• getDriveLetter now checks the return value of GetCurrentDirectory
• Modified writeFileContent to accept _In_z PCTSTR instead of const TCHAR
• Modified writeLog to accept In_z PCTSTR instead of const TCHAR
• Since writeLog doesn't have a return value (no way of reporting errors), I modified it to throw an exception if it can't open the file.
• Modified purgeMenuItemString to accept In_z PCTSTR instead of const TCHAR
• purgeMenuItemString now throws an exception if it tries to write out of bounds
• Many modifications to wcharMbcsConvertor::char2wchar:
• It now accepts an In_z PCSTR const for the string it is to convert
• int *pLenWc is now _Out_opt_ int* const pLenWc
• int *pBytesNotProcessed is now _Out_opt_ int* const pBytesNotProcessed
• Properly initializes pLenWc & pBytesNotProcessed when returning early
• I wrapped it in #pragma strict_gs_check(push on) and #pragma strict_gs_check(pop) • I annotated It's success return value • A few of the same modifications to WcharMbcsConverter::char2wchar (the one with mstart and mend) • A few of the same modifications to WcharMbcsConvertor::wchar2char • A few of the same modifications to WcharMbcsConvertor::wchar2char (the one with mstart and mend) • I wrapped string2wstring in#pragma strict_gs_check(push on)and#pragma strict_gs_check(pop)
• I stuck converFileName in an anonymous namespace
• I changed the name of PathRemoveFileSpec( generic_string & path ) to DynamicPathRemoveFileSpec
• OVERLOADING A SYSTEM FUNCTION - ONE THAT HAS A MACRO FOR A NAME - IS A VERY BAD IDEA!
• I annotated getCtrlBgColor, checked all the inner functions' return codes, and refactored it slightly so that it doesn't look like a giant arrow
precompiledHeaders.h:
• I added a few more headers that I needed, and moved a few out of implementation files (where they were frequently included)
• <array>
• <locale>
• <stack>
• <iterator>
• <type_traits>
• <TCHAR.h>
MiniDumper.cpp:
• I zero initialized the buffer
• I replaced lstrcat (that evil bastard) with tcscat
• NOTE TO SELF: change _tcscat to _tcscat_s
PluginsManager.cpp:
• There were a few suspicious accesses to _pluginInfos. So I swapped operator[] for .at()
• Set up automatic memory management for "local" heap variables
• When we're ready to transfer the automatically-managed heap variables to a vector of pointers, I release it
• Checked for out-of-bounds writes, and throw a std::out_of_range in that event
• Swapped lstrcpy (that evil bastard) with _tcscpy
• NOTE TO SELF: change _tcscpy to _tcscpy_s
• Swapped lstrcpyn (that evil bastard) with _tcsncpy_s
• I changed the catch-std::exception-by-value to by-reference (to avoid slicing), in several places
• Swapped generic_sprintf with _stprintf_s, in several places
Process.h/Process.cpp:
• I moved the definition of Process::Process from the header to the implementation file
• I annotated Process::Process
• I marked Process::run as const
regExtDlg.cpp:
• I swapped no-initialization for TCHAR buffers to zero initialize, in a bunch of places
Notepad_plus.cpp/Notepad_plus.h
• I stuck all the static/file-level data - the docTabIconIDs, the toolBarIcons - into an anonymous namespace
• Removed the pointer-checks-for-null before calling delete. The delete function/operator is guaranteed by the standard to handle a null pointer. Checking for null also is a cause of code bloat, and increases complexity.
• I also set all the pointers to nullptr, just because it's a good idea.
• I set _pMainSplitter to nullptr after deleting it
• I replaced the C-style cast in Notepad_plus::saveDockingParams with a const_cast
• I rearranged a few of the std::vectors in Notepad_plus::saveDockingParams so that I could call reserve
• I made sure that there was as much const as possible in Notepad_plus::saveDockingParams
• I changed the assignment of vPluginDockInfo to nppGUI._dockingData._pluginDockInfo from a std::vector<PluginDlgDockingInfo>& operator=(const std::vector<PluginDlgDockingInfo>& in ) to a std::vector<PluginDlgDockingInfo>& operator=(const std::vector<PluginDlgDockingInfo>&& in ) - a.k.a. copy-assignment to move-assignment
• I moved Notepad_plus::saveUserDefineLangs from the header to the implementation
• I moved Notepad_plus::saveShortcuts from the header to the implementation
• In Notepad_plus::getHtmlXmlEncoding, I swapped generic_fopen for _tfopen_s
• I moved Notepad_plus::getAccTable from the header to the implementation
• I moved Notepad_plus::getCurrentBuffer from the header to the implementation
• I moved a BUNCH of functions from the header to the implementation file….I don't want to list all of them!
• I swapped out a few manually managed buffers for std::unique_ptrs
• I swapped a lstrcpy in Notepad_plus::doSynScorll[sic] for _tcscpy_s
• I swapped a BUNCH of lstrcpy in the whole file for _tcscpy_s….I don't want to list all of them!
Notepad_plus_Window.cpp:
• I swapped wcstombs for wcstombs_s (which uses template magic!), and throw a std::runtime_error on failure
NppBigSwitch.cpp:
• Moved allowAppDataPluginsFile from header to implementation
• In SortTaskListPred::operator() I changed the catch clause from a capture std::exception by value to a capture by reference (to avoid slicing)
• A bunch of lstrcpy-and-friends functions swapped for safer equivalents
• For case NPPM_GETNPPDIRECTORY I now check the return value of GetModuleFileName, and in following the surrounding code, return TRUE. This needs some more work, in another patch.
• With regard to case NPPM_GETOPENFILENAMESPRIMARY, case NPPM_GETOPENFILENAMESSECOND, case NPPM_GETOPENFILENAMES:
• THEY'RE - in the word of Linus Torvalds - "terminally broken"
• There's no way to bounds check
• I don't think there's even a way to make sure the number of files open and the names of the open files stay the same in between the return of NPPM_NBOPENFILES
• How long is each path name?
• Honestly, I think we should depreciate this API and build a better one.
NppCommands.cpp:
• I pulled a bunch of the code from the switch/case into individual functions in an anonymous namespace. These are much easier to read/reason about
• I'm a total Nazi when it comes to braces in if/else clauses. Also, where I can't use strict Ratliff style, I at least try to be consistent - I put a ton of braces in this file. Even though I really REALLY hate putting the open/close braces on their own line, that seems to be the predominant style in NPP, so I follow it.
• There was a common chunk of code that converted an integer id to a "style" id, so I refactored it into its own function.
NppIO.cpp:
• I swapped a bunch of lstrcpy for _tcscpy
• I swapped a few localtime/generic_strftime calls for localtime_s/_tcsftime
• I added checks for the return value of GetModuleFileName
NppNotification.cpp:
• In Notepad_plus::notify, I swapped a raw char* heap allocation with a std::unique_ptr
• I swapped a bunch of lstrcpy for _tcscpy
• I commented out the blind try/catch construct - nothing in TTN_GETDISPINFO should throw an exception, and if something DOES then program state is certainly corrupted!
Parameters.cpp/Parameters.h:
• I moved all sorts of static/file-level data into an anonymous namespace
• Same thing with file-level functions
• I moved far more function definitions, from the header to the implementation file, than I can list
• In NppParameters::getCloudSettingsPath, I made SURE to check all of the HRESULT return codes
• In NppParameters::getCloudSettingsPath, I also commented out the blind try/catch construct - nothing here should throw a legitimate exception!
• I'm also now using a few std::unique_ptrs instead of manual memory management
• In NppParameters::getCloudSettingsPath, I didn't insert checks for EVERY single function call that can fail -> this needs work!
• In NppParameters::getCloudSettingsPath, I do throw a std::runtime_error form a few places where character conversion functions fail
• In NppParameters::destroyInstance, I removed the checks against null pointers before calling delete, and made sure that all deleted pointers are set to nullptr.
• In NppParameters::destroyInstance, I replaced some C-Style casts with reinterpret_cast
• In a large number of functions, I stuck const wherever I could
• In NppParameters::feedFindHistoryParameters, I made sure that we weren't re-using variables to hold return values. I've seen far too many bugs where a single variable (I'M LOOKING AT YOU, COM PROGRAMMERS!) is assigned the result of at least 80 function calls, and it loses meaning. It's then very hard to make sure that you're actually checking a given return code, and the static analysis tools start screaming at you for being an idiot. Nobody likes being screamed at.
• No, seriously, I put a ton of const in this file!
• In NppParameters::writeSession, I replaced a few C-Style casts with const_cast
---> As an aside, it's very hard to even discuss changes when functions are several hundred lines long. I can't just say "in stupidNamespace::goddamnedFunction, I did [some shit]…"
• There are a bunch of places wher I added checks against the return value of TiXmlElement::Attribute
• There's one line of code (ok, actually it's four "lines") where somebody went batshit crazy with the ternary operator. I can't even mentally parse this - my head explodes every time. Scraping my mind off the walls takes a very long time! Someone needs to figure this one out, because I can't.
• In the header, I annotated the array data members with _Field_size_part( NB_MAX_NUMBER_SOMETHING, _nbSomething ) to aid static analysis.
• In the header, I also switched a few raw arrays for std::array - much easier to work with! The software for the F-35 uses these exclusively over raw arrays; they're much easier to work with, much safer, and generally better. The F-35 guys apparently really like std::array!
• These two files are like fifty times too big!
AutoCompletion.cpp:
• I actually moved a few function definitions out of Parameters.h into an anonymous namespace here - I don't know how they ended up in the header in the first place…
• Careful annotation of AutoCompletion::getCloseTag picked up a bug, in that if we return early - say it's because targetStart == -1 or targetStart == -2, we'd fail to NULL terminate closeTag. closeTag is then passed to a function which expects a null terminated string, and somewhere, a small child cries. I fixed the issue, and thus saved a small child from misery.
• In AutoCompletion::setLanguage, I inserted checks for the return value of GetModuleFileName, and return (false) early where we hit an error.
---> As another aside, I've noticed that GetModuleFileName is misused frequently. I should probably set up a more systematic way of doing it.
Buffer.cpp:
• I stuck EventReset in an anonymous namespace. Compilers really like it when an object is in an anonymous namespace.
• I inserted a bunch of checks into EventReset, and added some debug tracing. The debug tracing was the result of a deadlock that I introduced while tweaking this file. Don't worry, it's fixed!
• I also replaced localtime with localtime_s
• In Buffer::getCurrentLang, I added a check to bail out if there are no actual valid items
• In FileManager::~FileManager, I swapped one of the C++03 style iterator loops with a ranged for loop. Looks so much better.
• Added a ton of error code checks in FileManager::LoadFile
• I went nuts trying to figure out what the hell kind of SHE exception FileManager::loadFileData could possibly throw; of course, it was actually a C++ exception.
• I made a number of changes to this section (the one that was guarded with a __try)
§ It's now a C++ try block
§ Careful annotation of other functions detected that we might be using the result of failed encoding, so I now throw a std::runtime_error in that case
§ Instead of the blind throw, I now throw a proper std::bad_alloc exception
§ The catch clause is no longer a generic catch-all (no pun intended), I now catch ONLY the std::bad_alloc, intentionally ONLY that.
§ I tweaked the error message to be SLIGHTLY better
• Added error checking in FileManager::getBufferFromName
columnEditor.cpp:
• Moved crap into anonymous namespace
• Added automatic memory management
FindReplaceDlg.cpp:
• Removed null-check prior to delete
• Carefully annotated FindReplaceDlg::ProcessFindNext
• Added error checking
• Added automatic memory management
FunctionCallTip.cpp/FunctionCallTip.h:
• Moved file-level structs and functions into anonymous namespace
• Moved a bunch of function definitions into implementation file
• Removed null-check prior to delete
GoToLineDlg.h:
• Class GoToLineDlg is now marked final
• display is now marked override
ScintillaEditView.cpp/ScintillaEditView.h:
• Moved file-level functions/data into an anonymous namespace
• Added more useful error handling/checking for getSciLexerFullPathName
• Removed unnecessary backslashes
• Switched a c-style cast to SCINTILLA_PTR/SCINTILLA_FUNC for a reinterpret_cast
• Added annoyed comment tobeginning of ScintillaEditView::scintillaNew_Proc
• We're NOT using C89! You don't have to declare variables at the top of the scope! We have const!
• Grr.
• Moved a bunch of function definitions into implementation file
• You've probably noticed that I have an unhealthy obsession with const - and yes, treatment helps.
• Swapped some scattered uses of itoa with _itoa_s
• Isn't it great when templates infer things for you??
• Added some scattered static_asserts, to scream in case somebody makes a change that accidentally breaks assumptions
• In this file, there are a ton of randomly-inserted single spaces, at the beginning of a line. What the hell? So, a bunch of the changes are just removing a SINGLE space, across four or five lines.
• There are also a few scattered two, or three, space insertions, all over the place.
• A bunch of functions are wrapped in #pragma strict_gs_check(push, on) and #pragma strict_gs_check(pop)
• This file is ALSO far bigger than it really should be.
• CharUpper/CharLower are really pretty terrible APIs.
UserDefineDialog.cpp:
• This file is almost exclusively formatting changes, so the important changes are hard to spot!
• In CommentStyleDialog::setKeywords2List, I replaced generic_itoa & lstrcpy with _itot_s & _tcscpy
• Similar changes in SymbolsStyleDialog::updateDlg
• Similar changes in SymbolsStyleDialog::setKeywords2List

I need to confess a few sins (I'm not even religious) I edited the TinyXml sources. Yes, I know, editing library files is heresy - but it pays off. The static analysis benefits are just too great!
• I annotated the crap out of far too many files to mention
• I added override and final in many places
• I added const everywhere
• I swapped all the dangerous functions with their bounds-checking counterparts
TinyXml is actually a great example of what SAL is capable of - as an API, SAL can diagnose many different kinds of misuse, that a pure static analyzer never could

Going forward:
• Systematic error handling?
• SAL wrappers for CloseHandle and friends?

(reserved) Major patch, now compiles cleanly without...

Notepad++ project is moving to GitHub:

Group

Searches

Help

#662 (reserved) Major patch, now compiles cleanly without _CRT_SECURE_NO_WARNINGS, and many other improvements

Discussion