- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Ehm, I'm confused, shouldn't it be memset(ptr, value, size) and value is literally a byte, so instead of NULL it should be 0?
Addendum 2024-04-22 07:04: To clarify, NULL is usually defined as something like ((void*)0), so the compiler should throw an error, because value is an integer, not a pointer for all versions of the C lib I have seen.
Admin
yeah. i hate people who use NULL to mean NUL.
A better way of clearing is memset(buff, 0, sizeof(buff)) (assuming buff is locally declared, and not a pointer that has been passed in, in which case you are likely to have different problems and CVE numbers attached to your code in the fullness of time).
Addendum 2024-04-22 07:24: There's no requirement for the compiler to throw an error when you pass NULL to memset though. It's a pointer, you can treat it as an unsigned char if you like. You might get a compiler warning. If you're lucky.
Admin
MFC application.. therefore C++ (not C) and therefore non of the raw string should be used. At the very least, use the templated versions that properly detect the size.
Admin
And this, boys and girls, is why you memset arrays using sizeof(array), so you can never get the size wrong like this. It should be:
Admin
Isn't it simpler to just use string constants in the OpenSaveFile call instead of szFileFilter, szDlgTitle and szDefExt? As far as I can tell, these are input only. The only array that should get updated is szFileName.
Admin
Pretty sure that line won't compile correctly. Try adding a double-quote mark before the last closing parenthesis.
Also, wouldn't passing a const char* defined string be better than doing a string copy for something that never changes? This seems to just waste memory, and is something we avoid in the embedded microcontroller world.
Admin
The author did not completely understand file select boxes in MFC. As the documentation for the OPENFILE-structure states, the lpStrFilter member is expected to point to pairs of two zero terminated strings in a row, finally terminated by two null bytes. This is not the same as a simple string concatenation. The first string would be the description, the second one the actual file mask like *.log. This is also the reason for the "strlen(...) + 1" in determining szPos, which leaves a nullbyte there, if it were properly prefilled with zeros (which it might not be because of the MAX_FILE_NAME/MAX_FILTER_SIE mismatch).
sizeof(sz) is nonsense nevertheless of course.
Admin
It's MFC, why aren't they using MFC's CString?
Admin
Because MFC is only a very thin wrapper for the Windows API and the Windows API for file select boxes is even worse than C Strings, requiring multiple zero terminated strings chaind up in memory terminated with two null bytes. Google the documentation for OPENFILENAME.lpstrfilter to see a really bad design choice.
Admin
Find one egregious error and you can be sure to find plenty more.
Admin
In C, sure. But this is C++ (because MFC, as TheCPUWizard says, but also because there's a
std::ifstream). In C++, NULL must be defined as just0. This is because it must implicitly convert to any pointer type, which((void*)0)does not. This has, of course lead to a whole category of bugs (given an overloadedvoid f(char*); void f(int);I hope you are ready forf(NULL);to call the int version) which is probably why we now have nullptr.Admin
Wait, int x = ((void*)0); doesn't throw an error?
Addendum 2024-04-22 12:42: Tried it and least that online c compiler only throws a warning :O
Admin
Isn't sizeof() also dangerous, because it doesn't work with alloced arrays? In that case sizeof returns sizeof(void*) or do I remember that incorrectly as well?
Addendum 2024-04-22 12:48: Tested it with:
Outputs:
So sizeof() is super dangerous as well.
Admin
Yes. That is how it should be done. Or if you really want a variable, say for documentation or reuse, you can do:
Admin
Also, szPos is off by 1 so the filter glob isn't appended to the string, even in the 64 bit build.
Admin
The memset()s are a red flag. They show that the programmer has a very poor understanding of strings. They also neatly illustrate the principle that adding unnecessary code provides more opportunities for bugs.
We also have a DRY principle violation - using the same literal text in the first copy and the subsequent strlen().
We have the bug that nobody else seems to have noticed of adding 1 to the result of strlen() so that the "*.log" is written after the "\0" and so is not seen by the called function. This is due to reinventing strcat() using strlen() + strcpy().
And, of course, we have the fine irony that using the "safe" string manipulation functions introduced a crash that would not have been there otherwise. Note that the code "suddently started working", but was only doing so by defeating the "safety" of strcpy_s() by passing a bopus length.
Admin
That is the essential failure of C++ that destroyed it as an application language: doing the wrong thing with memory is too easy, even when doing the right thing is also easy.
Admin
Why use memset at all ? Use initializer and then all elements are initialized by default value ( like static variables, always with 0)
char szFileName[MAX_FILE_NAME] = {}; char szFileFilter[MAX_FILE_NAME] = {};
Admin
I was trying to say something like that, but after 24 hours... Comment still held for moderation. I think that is the real WTF on this site.
Admin
Classic C errors: Getting confused by your own (Hungarian) notation/variable names/assumptions, and not paying attention to actual types; using sizeof() for string/byte operations; getting too deep into complex strcpy operations to begin with (and not using strncpy... at least they are using strcpy_s?); being required by the annoying MFC file filter string pattern list format (which has been inherited/copied by Qt as well unfortunately) to do any of this at all.
Admin
It's always neccesary to turn off or ignore warnings when writing code like this :)
Admin
sizeof() isn't dangerous. Your results make sense.
The type of x is a char pointer, which is the same size as a void pointer. The type of y is a char[2] - an array of 2 chars, which on most architectures is 2 bytes. That is not the size of a pointer of most architectures (usually 32 or 64 bit).
The only thing you really need to be careful is making sure you're taking the size of the item you're thinking of. Don't confuse a pointer with an array - in C they are related types to be interchangeable, but they aren't the same thing.
Admin
Warnings are for weak programmers. Real programmers write everything with void pointers and never get it wrong!
Real programmers also work 20-hour days, of course. And have no life outside of programming; the main reason they don't leave the company is that it would mean having to spend time not programming to find a new company.
Admin
I'm an idiot.