• (nodebb)

    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.

  • (nodebb)

    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.

  • TheCPUWizard (unregistered)

    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.

  • (nodebb)

    szFileFilter is declared using the size MAX_FILE_NAME, but when set to null, a space equal to MAX_FILTER_SIZE is used. If MAX_FILTER_SIZE is less than or equal to MAX_FILE_NAME this is fine- but if it's ever larger- welp, we've got an out of bounds access.

    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:

    memset(szFileFilter, 0, sizeof(szFileFilter));
    memset(szFileName, 0, sizeof(szFileName));
  • Rob (unregistered)

    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.

  • Brian Boorman (unregistered)
    strcpy_s(szFileFilter, sizeof(szFileFilter), "*.log - Debug Log File|*.log);

    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.

  • TopTension (unregistered)

    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.

  • Michael Kohne (unregistered)

    It's MFC, why aren't they using MFC's CString?

  • TopTension (unregistered) in reply to Michael Kohne

    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.

  • Argle (unregistered) in reply to Michael Kohne

    Find one egregious error and you can be sure to find plenty more.

  • Smithers (unregistered) in reply to MaxiTB

    NULL is usually defined as something like ((void*)0)

    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 just 0. 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 overloaded void f(char*); void f(int); I hope you are ready for f(NULL);to call the int version) which is probably why we now have nullptr.

  • (nodebb) in reply to thosrtanner

    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

    main.c:13:13: warning: initialization of ‘int’ from ‘void *’ makes integer from pointer without a cast [-Wint-conversion]
       13 |     int x = NULL;
  • (nodebb) in reply to Steve_The_Cynic

    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:

    void main()
        char *x = "X";
        char y[2] = "Y";
        printf("x: %d\n", sizeof(void*) == sizeof(x));
        printf("y: %d\n", sizeof(void*) == sizeof(y));


    x: 1
    y: 0

    So sizeof() is super dangerous as well.

  • (nodebb) in reply to Rob

    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.

    Yes. That is how it should be done. Or if you really want a variable, say for documentation or reuse, you can do:

    const char *szFileFilter = "*.log - Debug Log File|*.log";
  • jholdn (unregistered)

    Also, szPos is off by 1 so the filter glob isn't appended to the string, even in the 64 bit build.

  • (nodebb)

    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.

  • Duke of New York (unregistered) in reply to Michael Kohne

    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.

  • ismo (unregistered)

    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] = {};

  • Brian Boorman (unregistered) in reply to Gearhead

    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.

  • TF (unregistered)

    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.

  • TF (unregistered) in reply to MaxiTB

    It's always neccesary to turn off or ignore warnings when writing code like this :)

  • Worf (unregistered) in reply to MaxiTB

    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.

  • (nodebb) in reply to TF

    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.

  • jholdn (unregistered) in reply to jholdn

    I'm an idiot.

Leave a comment on “Concrapenate Strings”

Log In or post as a guest

Replying to comment #:

« Return to Article