OrangeC
OrangeC copied to clipboard
LADSoft
Run passes over OrangeC with AddressSanitizer
With VS2019 now being the new standard we compile to, a feature called AddressSanitizer can be used so that we can correct the programs functionality, such as in osutil.cpp we have:
void outputfile(char* buf, const char* orgbuf, const char* ext)
{
if (buf[strlen(buf) - 1] == '\\')
{
const char* p = strrchr(orgbuf, '\\');
if (p)
p++;
else
p = orgbuf;
strcat(buf, p);
Utils::StripExt(buf);
Utils::AddExt(buf, ext);
}
else if (prm_output.GetExists())
{
Utils::AddExt(buf, ext);
}
else
{
setfile(buf, orgbuf, ext);
}
}
Where the strcat and AddExt calls can overrun our buffer. This provides a great way for us to whittle out undefined behavior in the code, maybe at the cost of performance, but will guarantee correctness of the entire codebase at large,
Maybe it is reasonable to have the AdressSanitizer active in the debug builds only, and to also have a CI environment running the tests with that.
I believe it would work, and we can have ASAN during CLANG & MSVC stuff, however, it should be split into a different build setup as the way to debug them as far as I know are.... painful since VS (at least 2019) didn't have the best of support....
Began attempts at this, found stuff like utils.cpp where I don't understand what the algorithm is trying to achieve being caught, the way I did this was by modifying ms.mak to include a run for -fsanitize=address like so:
ifneq "$(MSPDB)" ""
CCFLAGS =/Od /Zi /FS /EHs /c /nologo /MTd /fsanitize=address
else
CCFLAGS = /O2 /EHs /c /nologo /MT
endif
And just defined MSPDB when running DEBUGCompiles.bat
Currently getting caught is:
std::string Utils::QualifiedFile(const char* path, const char* ext)
{
char buf[260];
Utils::StrCpy(buf, path);
char* p = strrchr(buf, '.');
if (!p || p[-1] == '.' || p[1] == '\\')
p = buf + strlen(buf);
Utils::StrCpy(p, sizeof(buf) - (p - buf), ext);
return std::string(buf);
}
Presumably because of the negative buffer access on p...
@LADSoft I put the code for QualifiedFile in here already, but I just want to check:
That !p || p[-1] == '.' check is invalid because if p is not nullptr, this whole thing short-circuits, otherwise, we access an invalid location and should be dead.
What is going on in that if statement to have it function? I need to start getting the Utils:: issues out of the way so I can verify if that access violation is a "real" access violation inside of #660
that assumes that p is either null or is pointing past the beginning of the buffer... if p is pointing to the beginning of the buffer then it is an invalid operation and may do an access violation... usually when I write code like that I've already incremented p but would have to see the specific instance to be sure...
missed your earlier post... that code seems to be looking for either ".." or "./".
the if should probably be written like this to avoid problems at the beginning of the buffer:
if (!p || (p != buf && p[-1] == '.') || p[1] == '\\')
The other question in that case is if the whole !p thing needs to be checked, as if p returns null there's no . at all within the string, I know generally we don't encounter this situation but if we do I want ASAN to not be complaining here. Should I change that to an && and then if there's nothing there convert it all to an std::string and just tack on the ending .whatever?
ok so i guess if you handle the !p check separately then do the other stuff only if it passes you would be good? Another approach is to convert the whole thing to a string up front then do the checks using string.find. Might even be better to transition the whole thing to take a string as an argument, but I don't know how much you want to bite off here. It would be sufficient just to handle the !p check separately from the rest... sometimes code has to expand a little bit to make analyzers happy...
Well, it seems the analyzer was happy with your suggestion, so my original theory is correct. I'm running into other analyzer situations, but not the one I originally wanted to debug so far :/ so I'm going to sidestep this issue for now and just use a VS compiled version with ASAN and simply place it where the build system likes. There ARE issues cropping up in the build system because of this (in the current build I just saw an access violation pass by), so my original purpose here was killed by the fact that the ASAN call did not trigger, meaning something is likely compiler-based causing my issue over in #660 now.