Codethulu's evil spawn, in fewer lines than you can imagine



  • Another C++ WTF from the same folks that brought you the last few WTFs of mine (names changed quite heavily to protect the [s]innocent[/s]guilty):

    [code]
    // PretendDatabase.H
    #define MAX_SERVICES 9000
    #define MAX_FUNCTIONS 9000

    class PretendDatabase
    {
    static bool isLoaded;
    static PretendDatabase **Services;
    static PretendDatabase **Functions;
    public:
    static bool Load();
    };

    // PretendDatabase.C
    bool PretendDatabase::isLoaded = false;
    PretendDatabase **PretendDatabase::Services = 0;
    PretendDatabase **PretendDatabase::Functions = 0;

    bool PretendDatabase::Load()
    {
    if( isLoaded )
    {
    return false;
    }

    isLoaded = true;
    Services = new PretendDatabase *[MAX_SERVICES];
    Functions = new PretendDatabase *[MAX_FUNCTIONS];
    
    // Initialize the arrays to NULL
    memset(Services,(sizeof(PretendDatabase *)*MAX_SERVICES),0);
    memset(Functions,(sizeof(PretendDatabase *)*MAX_FUNCTIONS),0);
    

    #include <PretendDatabaseFunctionsBuilder.h>

    return true;
    

    }

    [/code]

    Is the larger WTF:

    1. That it's including a header in the middle of a function,
    2. That it is newing fixed sized arrays that could simply have been allocated statically,
    3. That it initializes none of the memory it says it does,
    4. Or that it is part of a mechanism that is equal parts fake database (backed by flat text files & ingested by a hand-written parser that makes Discurse's regex soup look amazing in comparison, but that's a WTF for a whole another day) and poorly designed inter-process communication layer (that spams everybody with everybody else's messages, leading to astonishing amounts of network load)?


  • I vote for the clbuttic memset() fuckup. 💗



    1. Using C++ after 1999


  • @tarunik said:

    bool PretendDatabase::Load()
    {
    if( isLoaded )
    {
    return false;
    }

    If isLoaded = false, looks like it tries to load the database. However, if isLoaded = true, it doesn't unload anything either. Just returns false while leaving everything in a supposedly loaded state.

    Nice.

    Never mind...I've been doing this audit far too long today, as I seem to have touched the only part of this code that is NOT WTFy.



  • @chubertdev said:

    Using C++ after 1999 ever.

    FTFY,



  • This code even has undefined behaviour. At least; it will when you compare these values with NULL/nullptr. See, it seems the coder's trying to initialise a NULL array, but NULL does not need to be stored as "0" in memory. It probably does on any non-embedded system, though, but it may not if 0 is a valid address and there's no virtual memory...


  • Discourse touched me in a no-no place

    @SpoofedEx said:

    This code even has undefined behaviour. At least; it will when you compare these values with NULL/nullptr. See, it seems the coder's trying to initialise a NULL array, but NULL does not need to be stored as "0" in memory. It probably does on any non-embedded system, though, but it may not if 0 is a valid address and there's no virtual memory...

    Ah, but NULL is actually defined to be 0 by the standard. If the real not-point-to-anything “pointer” is something else, the type-cast machinery has to handle this special case right. Funnily enough, everyone actually uses 0. They just don't guarantee that it won't point somewhere…



  • @dkf said:

    Ah, but NULL is actually defined to be 0 by the standard. If the real not-point-to-anything “pointer” is something else, the type-cast machinery has to handle this special case right. Funnily enough, everyone actually uses 0. They just don't guarantee that it won't point somewhere…

    I think @SpoofedEx is referring to the (somewhat failed) attempt at using memset() to initialize pointers. I guess he's technically right there - memset() would fill the memory with the literal value 0 byte-for-byte, not with whatever magic pattern NULL could be using.

    Of course, the point is moot, because the code attempts to initialize the zero first bytes of the pointer arrays with whatever the value of sizeof(ptr)*9000 ends up being (interpreted as a byte), so it fails terribly either way.

    I've never had to work with an insane arch where NULL doesn't use zero as a pattern, but it seems that on those platforms you're not able to use memset() to initialize anything containing pointers... Interesting.

    But, yeah, definitively not the main problem with this code.


  • Discourse touched me in a no-no place

    @cvi said:

    I've never had to work with an insane arch where NULL doesn't use zero as a pattern

    I've never actually heard of a real example where someone was that crazy. I've seen it postulated as a theoretical example, but that's just pointless pedantic dickweedery if nobody does it. Even on thoroughly dopey architectures like old-school x86, NULL was still 0.


  • FoxDev

    @dkf said:

    I've never actually heard of a real example where someone was that crazy.

    Maybe no modern arch, but some historic ones used nonzero NULLs.



  • Obvious troll is obvious.



  • Hmmm, some people can't tell the difference between trolling and sarcasm.



  • @dkf said:

    I've never actually heard of a real example where someone was that crazy. I've seen it postulated as a theoretical example, but that's just pointless pedantic dickweedery if nobody does it. Even on thoroughly dopey architectures like old-school x86, NULL was still 0.

    C++ (and I think C as well) guarantee that 0 cast to a pointer type will produce an appropriately useless address. There are quite a few embedded systems where there exists valid memory at address 0, though...so writing to a NULL pointer is not always the segfault people coming from the PC world expect it to be.



  • @chubertdev said:

    Hmmm, some people can't tell the difference between trolling and sarcasm.

    Why can't it be both? Sarcastic trolling should earn extra points.



  • @cvi said:

    Of course, the point is moot, because the code attempts to initialize the zero first bytes of the pointer arrays with whatever the value of sizeof(ptr)*9000 ends up being (interpreted as a byte), so it fails terribly either way.

    Looks like they reversed the order of memset args. Whoops.


  • Discourse touched me in a no-no place

    @dkf said:

    Ah, but NULL is actually defined to be 0 by the standard.

    To molest a dead equine..

    NULL is specifically defined to not necessarily be all-bits-zero. 0 in a pointer context is NULL, but that doesn't mean the underlying representation, after compilation is all bits zero.

    char* p = 0; // Is NULL, need not be all-bits zero.
    memset(&p, sizeof p, 0); // may not result in a NULL pointer
    


  • @PJH said:

    To molest a dead equine..

    NULL is specifically defined to not necessarily be all-bits-zero. 0 in a pointer context is NULL, but that doesn't mean the underlying representation, after compilation is all bits zero.

    Many architecture ABIs, AIUI, do make this guarantee (for instance: AAPCS for the ARM does indeed guarantee NULL is all-bits-zero, which is interesting because ARM Cortex-Ms generally have valid memory at address 0).


  • Discourse touched me in a no-no place

    @tarunik said:

    Many architecture ABIs, AIUI, do make this guarantee (for instance: AAPCS for the ARM does indeed guarantee NULL is all-bits-zero, which is interesting because ARM Cortex-Ms generally have valid memory at address 0).

    Indeed. But we're playing Language Lawyer with the C Standard(tm) here. All 20-odd of them...


  • Discourse touched me in a no-no place

    Oh, and the following is dodgy..

    int null=0;
    char* c = null; // not guaranteed to == 0.
    


  • @tarunik said:

    Another C++ WTF from the same folks that brought you the last few WTFs of mine (names changed quite heavily to protect the [s]innocent[/s]guilty):

    [code]
    // PretendDatabase.H
    #define MAX_SERVICES 9000
    #define MAX_FUNCTIONS 9000

    class PretendDatabase
    {
    static bool isLoaded;
    static PretendDatabase **Services;
    static PretendDatabase **Functions;
    public:
    static bool Load();
    };

    // PretendDatabase.C
    bool PretendDatabase::isLoaded = false;
    PretendDatabase **PretendDatabase::Services = 0;
    PretendDatabase **PretendDatabase::Functions = 0;

    bool PretendDatabase::Load()
    {
    if( isLoaded )
    {
    return false;
    }

    isLoaded = true;
    Services = new PretendDatabase *[MAX_SERVICES];
    Functions = new PretendDatabase *[MAX_FUNCTIONS];
    
    // Initialize the arrays to NULL
    memset(Services,(sizeof(PretendDatabase *)*MAX_SERVICES),0);
    memset(Functions,(sizeof(PretendDatabase *)*MAX_FUNCTIONS),0);
    

    #include <PretendDatabaseFunctionsBuilder.h>

    return true;
    

    }

    [/code]

    Is the larger WTF:

    1. That it's including a header in the middle of a function,
    2. That it is newing fixed sized arrays that could simply have been allocated statically,
    3. That it initializes none of the memory it says it does,
    4. Or that it is part of a mechanism that is equal parts fake database (backed by flat text files & ingested by a hand-written parser that makes Discurse's regex soup look amazing in comparison, but that's a WTF for a whole another day) and poorly designed inter-process communication layer (that spams everybody with everybody else's messages, leading to astonishing amounts of network load)?

    This code should be a poster for how NOT to code, with all details listed in this thread used to demonstrate why. (Makes for educational trolling?)



  • @tarunik said:

    2. That it is newing fixed sized arrays that could simply have been allocated statically,

    Sometimes I've had to do that just because the array would sometimes be larger than the 12MB that Linux would allocate for the stack. TRWTF is that Linux still doesn't transfer memory from heap to stack when needed.



  • TRWTF is that the stack and the heap are controlled by the programmer instead of the compiler.

    <cough Go is best language cough>


  • @tarunik said:

    That it's including a header in the middle of a function,

    There are very rare cases this where that's not completely a wtf. One is if PretendDatabaseFunctionsBuilder.h is automatically-generated: it can be easier/nicer to generate just the bulk of something instead of generating the trappings around it.

    @tarunik said:

    That it initializes none of the memory it says it does,
    Incidentally, this is why I much prefer some ZeroInitialize/bzero type function to memset for that. A lot of people say that bzero is redundant, but I think it isn't because it makes that mistake almost impossible.

    @chubertdev said:

    Using C++ after 1999
    I have a strong love-hate relationship with C++, but even to the extent I dislike it I think 1999 is rather too early. At that point, what alternatives were there? The sort of three main alternatives that I see even today are C, Java, and C#. And I would always choose C++ over C for any kind of project in any situation, save for when C++ tooling support was flat-out unavailable (or at least really awful). I might use different subsets of C++ in different situations, but there is basically no non-trivial program for which I would say "C is the better choice here." C# wasn't around for a couple years after 1999 (Wikipedia says 2002 for 1.0 and 2005 for 2.0). Java in 1999 was kinda crappy; they hadn't had enough time to get the JVM to a modern level, and generics weren't introduced until fairly late in 2005; and at least IMO, generics are when "using Java" stopped being a WTF. So really, that didn't leave a ton of alternatives: and my next choices would be a pretty radical departure from the imperative, C-like style.



  • @EvanED said:

    Incidentally, this is why I much prefer some ZeroInitialize/bzero type function to memset for that. A lot of people say that bzero is redundant, but I think it isn't because it makes that mistake almost impossible.

    Other solutions to this problem:

    • A standard library/runtime that zeroes out newly allocated memory automatically
    • A language that doesn't allow you to put a memory size value in a byte argument
    • using new() instead of new, so new PretendDatabase *[MAX_SERVICES]()

  • Discourse touched me in a no-no place

    @ben_lubar said:

    A language that doesn't allow you to put a memory size value in a byte argument

    This one is the real fix. A vast number of problems in C would have never occurred if that sort of type enforcement had been there all along, instead of sub-word-size arguments being a gross hack (arguments larger than a word aren't much better off either, FWIW).

    I still use C for some things though. It's just so much easier to get it built once the code is right; you typically only fight with compilers during active development. C++ is far more finicky. (I also use Java a lot for work, and avoid C# because of platform issues.)



  • @dkf said:

    (I also use Java a lot for work, and avoid C# because of platform issues.)

    What platform are you running code on that supports Java but not C#?


  • Discourse touched me in a no-no place

    @blakeyrat said:

    What platform are you running code on that supports Java but not C#?

    OSX and heavily serverized Linux. The quality of C# implementation there is a long way below where I believe it to be on Windows. It's better now, I suppose, but we didn't start the software last week. Or even this decade. We have a lot of legacy to deal with too.

    I try not to think about it too much at the weekend…



  • Oh so when you say "platform issues" you mean "there were platform issues a fucking decade ago, and now it's a legacy app."

    My own fault for assuming you might type something even remotely close to what you were trying to communicate.


  • ♿ (Parody)

    @EvanED said:

    There are very rare cases this where that's not completely a wtf. One is if PretendDatabaseFunctionsBuilder.h is automatically-generated: it can be easier/nicer to generate just the bulk of something instead of generating the trappings around it.

    Still smells like a WTF. Better then to refactor the generated stuff out and call the resulting function than to abuse the pre-processor like that.



  • @EvanED said:

    And I would always choose C++ over C for any kind of project in any situation, save for when C++ tooling support was flat-out unavailable (or at least really awful). I might use different subsets of C++ in different situations, but there is basically no non-trivial program for which I would say "C is the better choice here."

    C++ is OK for write-only, single-programmer code that never needs to be maintained (and especially not code that needs to be maintained at 3AM by someone who's never seen that particular part of the codebase before.)

    It's a fucking abomination.



  • @boomzilla said:

    abuse the pre-processor

    But that's what the pre-processor is for.


  • Discourse touched me in a no-no place

    Is it preprocessor abuse if you're using it to effectively generate new keywords in the language? I ask because I really like my #define foreach(…) macro…



  • @dkf said:

    Is it preprocessor abuse if you're using it to effectively generate new keywords in the language? I ask because I really like my #define foreach(…) macro…

    Not when the language already has it (Wiki)
    That's only since C++11 though.



  • @blakeyrat said:

    Oh so when you say "platform issues" you mean "there were platform issues a fucking decade ago, and now it's a legacy app."

    Ah, so any code more than four years old is without value and needs a ground up rewrite. Good to know that particular brainworm isn't limited to the OSS community.



  • @jello said:

    Ah, so any code more than four years old is without value and needs a ground up rewrite.

    That's not even remotely similar to what I typed.



  • @SpoofedEx said:

    dkf:
    Is it preprocessor abuse if you're using it to effectively generate new keywords in the language? I ask because I really like my #define foreach(…) macro…

    Not when the language already has it (Wiki)That's only since C++11 though.

    Discovered at this job that I can do the following in VS2008 (yeah, we still haven't updated...):

    std::vector<int> stuff;
    for each (int i in stuff) {}



  • @blakeyrat said:

    That's not even remotely similar to what I typed.

    Well...

    @dkf said:

    we didn't start the software last week. Or even this decade.

    i.e. 2010 or prior

    @blakeyrat said:

    now it's a legacy app.

    i.e. Outdated.

    @jello said:

    Ah, so any code more than four years old is without value and needs a ground up rewrite.

    @blakeyrat said:

    My own fault for assuming you might type something even remotely close to what you were trying to communicate.


  • Discourse touched me in a no-no place

    @dkf said:

    Is it preprocessor abuse if you're using it to effectively generate new keywords in the language? I ask because I really like my #define foreach(…) macro…

    Yes.


  • FoxDev

    I might possibly top the OP for this one.

    new Dictionary<string, Tuple<Func<string, Action<Task<QueueHandlerResult>>>, string[], string>>();
    

    ... what the..... nope. do not want to know!



  • @accalia said:

    I might possibly top the OP for this one.

    new Dictionary<string, Tuple<Func<string, Action<Task<QueueHandlerResult>>>, string[], string>>();

    ... what the..... nope. do not want to know!

    Still doesn't top #4 on [s]my list[/s]the list from the OP, though. (P.S. If I seem slow to respond, it's because I'm wrestling with said fake database.)


  • FoxDev

    @tarunik said:

    Still doesn't top #4 on my list,

    You're gonna share that list soon right?


  • Discourse touched me in a no-no place

    @PJH said:

    http://c-faq.com/cpp/slm.html

    Yes, but having macro sequences for things like that still helps. They should be short and small in number though. I particularly tend to do it for cases where otherwise there would be an annoying stanza that takes quite a bit of stereotypical code to write: iterating over a list, array or hashtable are the cases where they really help (you can get the code right and then stop worrying about whether all the places that you need it are correct). My aim with those is to have a construct that can be used with a standard block so that the rest of the code isn't strange.

    Beware of anyone who wraps up a goto or longjmp(): they are capable of anything.



  • @PJH said:

    To molest a dead equine..

    NULL is specifically defined to not necessarily be all-bits-zero. 0 in a pointer context is NULL, but that doesn't mean the underlying representation, after compilation is all bits zero.

    char* p = 0; // Is NULL, need not be all-bits zero.
    memset(&p, 0, sizeof(p)); // may not result in a NULL pointer
    

    Fixed the order of arguments 2 and 3 in the memset.

    You're technically right though: your version of the memset line indeed would most likely not result in a NULL pointer ;)


Log in to reply