Ah, I think I know what the problem is, (it's my fault, not used to diff/patch).
I tried it myself, and patch failed, as it couldn't find the file. Duh.
I kept the modified version in a folder called patched, and the original in original, and diff stored this. Revert all changes made to the directory (if there were any), and try the attached patch. It's been patched (yeah, a patched patch).
The latest patch you submitted still doesn't work for me (the same error).
It seems to me the absolute path are used.
Have you tried to rename your folder before applying the patch?
Don
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
How are you applying the patch? It sounds like your using TortoiseSVN? If you're using good ol' patch, could you post the (failing) output with --verbose?
Yeah, I tried renaming the folder. Everything is good on my end.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yes, I'm using TortoiseSVN with GUI.
After applying your patch on directory "trunk", a "patch" directory is created with the sub-directory (see attached file), and they are all empty.
Example of summary:
1. Initialize the local character array with empty string: explanation...
2. use %u instead of %d when variable is unsigned int: explanation...
3. ...
The coding guideline is rather for me to avoid such errors in the future.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Actually, the two biggest things you can do are not even coding guidelines:
Use the built in visual studio code analysis tools; because vc120_xp, for some weird reason, doesn't support code analysis, what I do is create a separate build configuration (e.g. "Unicode Static Analysis"), and build that config. You'll also need to disable precompiled headers for this config, else the compiler will crash/hit an Internal Compiler Error. This patch was the result of such a build.
Configure your own installation of Notepad++ to run under Application Verifier. Application Verifier cones with the Windows SDK and is a truly enlightening tool, which catches an incredible number of bugs at runtime, without recompilation/instrumentation. I've found bugs in everything from VLC & Git, to Notepad++ itself, this way. Open Application Verifier, add Notepad++ (under file), and in addition to the default checks, enable DirtyStacks (under miscellaneous), and in the properties for Heaps (under basics), enable: "Address", "Protect", "Size", & "LFH guard pages". Now, many bugs will show up immediately, at the offending instruction. For example: if Notepad++ overwrites a buffer (by more than ~4 bytes), it'll touch the next page, and trigger an access violation. If Notepad++ tries to use memory after freeing it, Application Verifier will trigger a debug break. If Notepad++ passes an invalid HANDLE to a system function, Application Verifier will trigger a debug break.
There are two downsides to using Application Verifier:
The instance of Notepad++ under test will suffer a performance hit; which won't be a real problem, because it's already very fast ;)
If it's not running under a debugger, Notepad++ will just crash. If you have full memory dumps enabled, opening in WinDbg & running !analyze -v will probably tell you exactly what the problem is.
Other good practices:
For C-style strings, prefer PTSTR/PCTSTR over TCHAR*/const TCHAR*. PTSTR has special meaning for the static analyzer - it means 'pointer to a null-terminated array of characters', which thus allows it to detect misuse of any standard-library functions that expect null terminated strings.
Any function that writes too a buffer (passed as a pointer), should require a parameter that describes the size of the buffer.
When writing string handling functions, in addition to #1, SAL annotate function parameters. I'll admit, SAL annotated code looks absolutely horrible at first, but it is infinitely useful for string/buffer handling. In fact, Microsoft invented it to detect and prevent string handling issues, which are (nearly always) security issues. For example, if you're writing a function that writes a null-terminated string to a buffer (passed as a pointer), use _Out_z_, or when following #2, use _Out_writes_z_( sizeOfBuffer ). Similarly, for a function that expects a pointer-to-a-null-terminated string as a parameter, and reads from it, mark the parameter _In_z_.
When writing functions that read-from/write-to a buffer, which accept a pointer to the first element, annotate that pointer to describe valid bounds. For reading from, the correct annotation is _In_reads_( sizeOfBuffer ). For writing to, it's _Out_writes_( sizeOfBuffer ).
Parameters & variables that describe the size of an object or buffer should use the type rsize_t. This one's actually a CERT standard.
For functions that return a success or failure code, annotate the successful condition. SAL has a handy annotation that greatly aids in detecting use of invalid _Out_ parameters, _Success_( someCondition ). So for a function that returns NULL on failure, the annotation would be _Success_( return != NULL ), and is placed before the functions return type.
Use const like you get paid every time you type those 5 characters. Ok, this isn't a string handling thing, I'm just a total const nazi.
Zero-initialize string buffers. Thus is not strictly necessary, but might limit the damage done by dangerous APIs.
never ever ever ever EVER use lstrcpy. Ever. Or any other function that blindly swallows SEH exceptions. Seriously. Not kidding about this.
Once again I'll admit, SAL can make your function declarations much longer, and uglier, but once you're past that, they're self-documenting, and a huge benefit to static analysis.
Last edit: Alexander Riccio 2015-03-16
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
A few other rules that I've thought of while writing patch 662.
continue list here:
Never ever ever ignore return values, even if the function looks like it should never fail. Especially if it looks like it should never fail. Checking return values is not optional! If program correctness was optional, then checking return values would be optional! If program correctness was optional, then we could make Notepad++ arbitrarily small, and arbitrarily fast!
Don't catch or throw SEH exceptions. They're almost never recoverable, and the best thing to do is crash & burn.
Don't MIX C++ and SEH exceptions. There's a particularly egregious example in FileManager::loadFileData - Notepad++ throws an "empty" C++ exception, INSIDE an SEH block. Figuring out what the hell was going on here was a real pain - an empty C++ exception is technically a call to std::terminate( ), but catching it with SEH seems to stop this. Even worse, this isn't any kind of known SEH exception, so I couldn't match it up with any documented SEH exception. I even wrote a filter that had a HUGE switch/case construct that handled every single known SEH exception, and it still fell through to the default case. Please don't do that to anybody - it's cruel!
Throw and catch ONLY the most specific exceptions! catch(...) is the worst offender. If you can't predict the exception, then there is certainly a bug! In #662, I changed FileManager::LoadFileData to throw (and catch) a std::bad_alloc on SC_STATUS_BADALLOC - which clearly documents intent - but also to throw a generic std::exception on SC_STATUS_FAILURE, which is NOT expected, and generally bad news. Stack unwinding is probably the best option on SC_STATUS_FAILURE.
...a few more to come tomorrow, it's time for bed!
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
As a sidenote: how many Notepad++ users actually run Notepad++ on Windows XP? I'd happily rewrite all of the string formatting functions to use their bounds-checked/Strsafe equivalents, but vc120_xp doesn't support any of them.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thank you Alexander,
I got REJECTED PATCH HUNKS message for each patch.
Any idea about that?
Don
Ah, I think I know what the problem is, (it's my fault, not used to
diff/patch).I tried it myself, and
patchfailed, as it couldn't find the file. Duh.I kept the modified version in a folder called
patched, and the original inoriginal, and diff stored this. Revert all changes made to the directory (if there were any), and try the attached patch. It's been patched (yeah, a patched patch).I've also attached the successful output of applying the patch.
The latest patch you submitted still doesn't work for me (the same error).
It seems to me the absolute path are used.
Have you tried to rename your folder before applying the patch?
Don
How are you applying the patch? It sounds like your using TortoiseSVN? If you're using good ol'
patch, could you post the (failing) output with--verbose?Yeah, I tried renaming the folder. Everything is good on my end.
Yes, I'm using TortoiseSVN with GUI.
After applying your patch on directory "trunk", a "patch" directory is created with the sub-directory (see attached file), and they are all empty.
I test with patch 633, works fine for me:
https://sourceforge.net/p/notepad-plus/patches/633/
And when I try to preview patched file. It shows me "Rejected patch hunks for ..." on the title bar (see attached file)
Last edit: Don HO 2015-03-14
Alexander,
After struggling during 1 hour with the patch, I quit.
Please attach your zipped source code, then I will merge manually.
Don
Hmm, it seems to understand the pat, as indicated by the screenshot, so I'm stumped there. Sorry to make you struggle. I'll post that zip file.
Alrighty, here it is.
Thank you Alexander. It's in SVN now.
Don
Last edit: Don HO 2015-03-15
Could you do a summary (+ small explanation) about your patch, which will be the guideline for coding Notepad++?
Thank you
Don
Yay!
A summary? Sure!
What exactly do you mean by coding guideline?
Example of summary:
1. Initialize the local character array with empty string: explanation...
2. use %u instead of %d when variable is unsigned int: explanation...
3. ...
The coding guideline is rather for me to avoid such errors in the future.
ad.2: Format Specification Fields and Type Field Characters
Nitpicking: Format specification fields
Nitpicking: Type field characters
Last edit: Alexander Riccio 2015-03-16
Actually, the two biggest things you can do are not even coding guidelines:
Use the built in visual studio code analysis tools; because vc120_xp, for some weird reason, doesn't support code analysis, what I do is create a separate build configuration (e.g. "Unicode Static Analysis"), and build that config. You'll also need to disable precompiled headers for this config, else the compiler will crash/hit an Internal Compiler Error. This patch was the result of such a build.
Configure your own installation of Notepad++ to run under Application Verifier. Application Verifier cones with the Windows SDK and is a truly enlightening tool, which catches an incredible number of bugs at runtime, without recompilation/instrumentation. I've found bugs in everything from VLC & Git, to Notepad++ itself, this way. Open Application Verifier, add Notepad++ (under file), and in addition to the default checks, enable DirtyStacks (under miscellaneous), and in the properties for Heaps (under basics), enable: "Address", "Protect", "Size", & "LFH guard pages". Now, many bugs will show up immediately, at the offending instruction. For example: if Notepad++ overwrites a buffer (by more than ~4 bytes), it'll touch the next page, and trigger an access violation. If Notepad++ tries to use memory after freeing it, Application Verifier will trigger a debug break. If Notepad++ passes an invalid HANDLE to a system function, Application Verifier will trigger a debug break.
There are two downsides to using Application Verifier:
!analyze -vwill probably tell you exactly what the problem is.Other good practices:
PTSTR/PCTSTRoverTCHAR*/const TCHAR*.PTSTRhas special meaning for the static analyzer - it means 'pointer to a null-terminated array of characters', which thus allows it to detect misuse of any standard-library functions that expect null terminated strings._Out_z_, or when following #2, use_Out_writes_z_( sizeOfBuffer ). Similarly, for a function that expects a pointer-to-a-null-terminated string as a parameter, and reads from it, mark the parameter_In_z_._In_reads_( sizeOfBuffer ). For writing to, it's_Out_writes_( sizeOfBuffer )._Out_parameters,_Success_( someCondition ). So for a function that returns NULL on failure, the annotation would be_Success_( return != NULL ), and is placed before the functions return type.constlike you get paid every time you type those 5 characters. Ok, this isn't a string handling thing, I'm just a total const nazi.Once again I'll admit, SAL can make your function declarations much longer, and uglier, but once you're past that, they're self-documenting, and a huge benefit to static analysis.
Last edit: Alexander Riccio 2015-03-16
A few other rules that I've thought of while writing patch 662.
continue list here:
std::terminate( ), but catching it with SEH seems to stop this. Even worse, this isn't any kind of known SEH exception, so I couldn't match it up with any documented SEH exception. I even wrote a filter that had a HUGE switch/case construct that handled every single known SEH exception, and it still fell through to the default case. Please don't do that to anybody - it's cruel!catch(...)is the worst offender. If you can't predict the exception, then there is certainly a bug! In #662, I changed FileManager::LoadFileData to throw (and catch) a std::bad_alloc on SC_STATUS_BADALLOC - which clearly documents intent - but also to throw a generic std::exception on SC_STATUS_FAILURE, which is NOT expected, and generally bad news. Stack unwinding is probably the best option on SC_STATUS_FAILURE....a few more to come tomorrow, it's time for bed!
For who is interested: Source-code Annotation Language (SAL)
+100 on using
constand NOT using thelstr-functions!Last edit: Menno Vogels 2015-03-16
Also relevant: https://randomascii.wordpress.com/category/code-analysis/
I should also qualify that my const-nazism really does border on straight up insanity. I mean, look at this crazy shit: https://github.com/ariccio/altWinDirStat/blob/master/WinDirStat/windirstat/TreeListControl.h#L315
Yes, those are const-pointers-to-const, on a const method.
Last edit: Alexander Riccio 2015-03-16
NICE!
LOL --> "macros_that_scare_small_children.h"
Yeah, I totally stole that from Google's C++ style guide :)
Side note: I learned a LOT by poking around the Chromium sources.
Last edit: Alexander Riccio 2015-03-16
As a sidenote: how many Notepad++ users actually run Notepad++ on Windows XP? I'd happily rewrite all of the string formatting functions to use their bounds-checked/Strsafe equivalents, but
vc120_xpdoesn't support any of them.