Jump to content

View more

Image of the Day

New ninja for my game.
#screenshotsaturday #gamedev #indiedev #IndieDevHour
https://t.co/sJdnPuiVgh
IOTD | Top Screenshots

The latest, straight to your Inbox.

Subscribe to GameDev.net's newsletters to receive the latest updates and exclusive content.


Sign up now

How to lose friends and alienate coworkers.

2: Adsense
  • You cannot reply to this topic
13 replies to this topic

#1 irreversible   Members   

2752
Like
0Likes
Like

Posted 10 February 2017 - 02:57 PM

This thread is semi-fictitious, because I don't have coworkers in my coding life. Nevertheless, I was trying to think of insidious ways to plant latent bombs in some otherwise cherished shared code (or your own, should you so desire!). Here are a few to get the ball rolling. Please feel free to post your own nefarious ideas!

 

The first two exploit the potentially single most abusive concept in coding I have in recent memory. To top things off, neither of these particular examples triggers a compiler warning in VS.

 

Do this: change your GetWidth() and/or GetHeight() functions to return uints overnight. Then get popcorn and wait for someone to do something like this:

set_screencoord(x - (GetWidth() >> 1), y - (GetHeight >> 1));

For context - I recently changed my window extent getters to uint32s. Suffice to say I'm seriously considering changing them back.

 

 

Or here's one of my favorites. Can go undetected for weeks, this one.

for(size_t i = 0; i < counter; ++i) { if(condition) { vec.quick_erase(vec.begin() + i); i--; } }

This one has gotten me a few times. The dirty bugger.

 

 

And finally something a little bit more evil. Something far more insidious, should you be naive enough to rely on simple behavior. I discovered this one in my memory manager the other day (although mine had nothing to do with clearing memory or anything that could be corruptive in any way, but rather had to do with reporting errors that led me to believe I had a serious cross-thread memory leak on my hands (which I didn't); that being said, I'd like to stress that the use of ZeroMemory() in this case is as illustrative as it gets!). Also, in my case I was dealing with smart pointers and a multithreaded memory manager, which obfuscated the issue quite a bit (although this has no real bearing on the problem at hand). What I'm trying to say is that don't judge the code - I've stripped the example down to something really really dumb and basic for reasons of clarity.

 

This one can be tough, but can you figure it out without further hints? The only assumption you should make is that allocations can be tightly packed, in that baseAddr2 = (baseAddr1 + size1 + sizeof(size_t)).

 

EDIT: a few more assumptions for the sake of clarity: assume that no (unintentional or otherwise) mistake or malicious manipulation of the allocated pointer has been made prior to calling mydelete(). Also assume that the compiler has not rearranged the members of allocrec_t - this is not the issue. Don't assume anything other than perfectly valid use of C++ in all its glory.

struct allocrec_t
{
   void* baseAddr;
   size_t size;
};
void mydelete(void * ptr)
{
   allocrec_t* ar = (allocrec_t*)ptr;

   ZeroMemory(ar->baseAddr, ar->size);
}

Edited by irreversible, 10 February 2017 - 03:12 PM.


#2 godmachine33   GDNet+   

115
Like
0Likes
Like

Posted 18 February 2017 - 12:49 PM

Remind me to never get on your bad side!



#3 Daixiwen   Members   

584
Like
1Likes
Like

Posted 20 February 2017 - 01:40 AM

How about adding a

#define TRUE FALSE
hidden deep in a header file somewhere?
Definition of a man-year: 730 people trying to finish the project before lunch

#4 TheComet   Members   

2802
Like
1Likes
Like

Posted 16 March 2017 - 12:42 AM

One way I shot myself in the foot and wasted days debugging was this:

#if defined(TESTING)
#   define protected public
#   define private public
#endif
Some libraries in the project were compiled with and without TESTING defined. Works on gcc for some reason but causes seemingly random runtime errors on Windows.

"I would try to find halo source code by bungie best fps engine ever created, u see why call of duty loses speed due to its detail." -- GettingNifty


#5 MarkS   Members   

3410
Like
0Likes
Like

Posted 19 March 2017 - 09:34 PM

This one has gotten me a few times. The dirty bugger.

struct allocrec_t
{
   void* baseAddr;
   size_t size;
};
void mydelete(void * ptr)
{
   allocrec_t* ar = (allocrec_t*)ptr;

   ZeroMemory(ar->baseAddr, ar->size);
}


OK, you win. I humble myself before you and admit my inadequacy as a programmer. You are the master and I am but a lowly hobbyist. :(

Now that the groveling is done.... What is wrong here? What am I missing? I've been banging my head against a wall for weeks!

#6 Nypyren   Members   

11706
Like
2Likes
Like

Posted 19 March 2017 - 11:26 PM

This one has gotten me a few times. The dirty bugger.

struct allocrec_t
{
   void* baseAddr;
   size_t size;
};
void mydelete(void * ptr)
{
   allocrec_t* ar = (allocrec_t*)ptr;

   ZeroMemory(ar->baseAddr, ar->size);
}


OK, you win. I humble myself before you and admit my inadequacy as a programmer. You are the master and I am but a lowly hobbyist. :(

Now that the groveling is done.... What is wrong here? What am I missing? I've been banging my head against a wall for weeks!


Not actually invoking free (depends on what that function is SUPPOSED to do, though. if it's pooling then that might be acceptable), not sanity checking ar->size (it could be uninitialized), passing void* instead of allocrec_t* bypassing any hope of type checking helping you not f*** up calls to it with the wrong type, etc, etc...

Edited by Nypyren, 19 March 2017 - 11:32 PM.


#7 Oberon_Command   Members   

5818
Like
1Likes
Like

Posted 19 March 2017 - 11:31 PM

This one has gotten me a few times. The dirty bugger.

struct allocrec_t
{
   void* baseAddr;
   size_t size;
};
void mydelete(void * ptr)
{
   allocrec_t* ar = (allocrec_t*)ptr;

   ZeroMemory(ar->baseAddr, ar->size);
}


1. Unless I'm missing something, shouldn't that be something like (allocrect_t*)((unsigned char *)ptr - sizeof(allocrect_t))?
2. With the code as written, if ptr == baseAddr (which I assume it is, which is why I ask the above question) the ZeroMemory will make ar->size be 0, so if the records are tightly packed the record will show as having a 0 size - so the ACTUAL record size will be lost on deallocation, which can cause all kinds of bugs.

Edited by Oberon_Command, 19 March 2017 - 11:34 PM.


#8 MarkS   Members   

3410
Like
0Likes
Like

Posted 20 March 2017 - 03:02 AM

Not actually invoking free (depends on what that function is SUPPOSED to do, though. if it's pooling then that might be acceptable), not sanity checking ar->size (it could be uninitialized), passing void* instead of allocrec_t* bypassing any hope of type checking helping you not f*** up calls to it with the wrong type, etc, etc...


giphy.gif

#9 matt77hias   Members   

278
Like
0Likes
Like

Posted 20 March 2017 - 03:36 AM

#if defined(TESTING) # define protected public # define private public #endif

 

And this is why we need a reflection API in C++^^



#10 l0calh05t   Members   

1733
Like
0Likes
Like

Posted 20 March 2017 - 04:54 AM

 

#if defined(TESTING) # define protected public # define private public #endif

 

And this is why we need a reflection API in C++^^

 

Nope. There are many good reasons for (static!) reflection in C++, but this sure as hell isn't one of them. private parts of a class are NOT part of the API. protected parts are only part of the API for things that derive from it. So you should never, ever, do this.



#11 matt77hias   Members   

278
Like
0Likes
Like

Posted 20 March 2017 - 06:56 AM

@l0calh05t

If one wants to write unit tests (whether to achieve full code coverage or not), one should include private methods as well. Except for their access modifier, they do not differ from any other methods! They have preconditions, postconditions, should be documented and ideally should be tested separetely (private methods are in fact more "unit" than public methods).

Lots of Java (or C#) codebases use reflection for this purpose:

Method method = targetClass.getDeclaredMethod(methodName, argClasses);
method.setAccessible(true);
return method.invoke(targetObject, argObjects);

Edited by matt77hias, 20 March 2017 - 06:57 AM.


#12 SillyCow   Members   

1386
Like
0Likes
Like

Posted 20 March 2017 - 07:13 AM

Why make it so predictable?

Random chaos is much more fun...

std::thread([]{
    while( true){
    //cause problems randomly, but only once every 15 minutes on average
    sleep(1000);
    if(0==rand()%(15*60)) {
        char *x=(void*)rand();
        //seg fault? just corrupt some data? who knows? let's make it a surprise...
        *x=rand();
    }
    }}

My browser game: Vitrage - A game of stained glass

My android games : Enemies of the Crown &  Killer Bees


#13 l0calh05t   Members   

1733
Like
0Likes
Like

Posted 21 March 2017 - 01:41 AM

 

@l0calh05t

If one wants to write unit tests (whether to achieve full code coverage or not), one should include private methods as well. Except for their access modifier, they do not differ from any other methods! They have preconditions, postconditions, should be documented and ideally should be tested separetely (private methods are in fact more "unit" than public methods).

Lots of Java (or C#) codebases use reflection for this purpose:

Method method = targetClass.getDeclaredMethod(methodName, argClasses);
method.setAccessible(true);
return method.invoke(targetObject, argObjects);

Nonsense. Private methods are not at all like other methods. They are implementation details! So those "pre and post conditions" are liable to change at any time, making your tests utterly worthless. A private method can even leave an object in an inconsistent state. DO NOT do this. And doing this to achieve better "code coverage" is even worse, because if any paths in private methods cannot be exercised via public methods, they are dead code and should be removed.

Or if you prefer a quote from a book, Dave Thomas and Andy Hunt write the following in Pragmatic Unit Testing:

In general, you don't want to break any encapsulation for the sake of testing (or as Mom used to say, "don't expose your privates!"). Most of the time, you should be able to test a class by exercising its public methods. If there is significant functionality that is hidden behind private or protected access, that might be a warning sign that there's another class in there struggling to get out.


#14 matt77hias   Members   

278
Like
0Likes
Like

Posted 21 March 2017 - 02:21 AM

Nonsense. Private methods are not at all like other methods. They are implementation details! So those "pre and post conditions" are liable to change at any time, making your tests utterly worthless. A private method can even leave an object in an inconsistent state. DO NOT do this. And doing this to achieve better "code coverage" is even worse, because if any paths in private methods cannot be exercised via public methods, they are dead code and should be removed.

Your arguments make no sense. The way private methods are implemented are implementation deals like any other method. That is you encapsulate the how or the imperative nature of your code. Thus treating them as black boxes like any other method both when using them and when testing them via unit tests. Both the use and the testing make no sense without pre- and postconditions which only expose the what or the declarative nature of your code. Without having those conditions, your methods should be treated as dead code since no one would have a clue what (not how) the methods do. Saying that the conditions change can mean several things:

  1. You refactored your code. In this case, documentation, pre- and postconditions need to be updated accordingly since you modified the declarative behaviour of some methods or added some new methods.
  2. You do not understand the difference between declarative and imperative, making you leak implementation details via the documentation, pre- and/or postcondtions. In this case you should do some research on how formal documentation works.

Leaving objects in an inconsistent state is not a problem, whether this occur in a public or private method, as long as it is documented. Therefore one uses code annotations (at class, methode, parameter level, etc.) for instance in Java or just enforce immutability at the language level like Haskell.

Of course not all languages (C++, etc.) provide support for testing private methods. That is a design decision of the laguage itself and irrelevant for the general discussion. (As a sidenote: in software design research one uses languages like Haskell, First and Second Order Logic languages like IDP, since one can enforce certain pre- and/or postcondtions in the code itself (at all levels independent of some access modifier). In practice for nuclear reactors for instance, one uses different languages in their toolset as well, since one wants to control all aspects of the code.)

Finally, it is important to realize that one library of a software project has multiple APIs. You have the API you expose to the end user who is only concerned with using the code (not testing the code). The API is also exposed to API programmers which have access to all code independent of their access modifiers. The access modifier only restricts the usage range. So different API programmers may use the same private method inside the same class. If they, however, have no guarantees that the method works as intended, they could not build upon the functionality. They could detect inconsistencies by testing public methods using those private methods, but could not deduce whether the public method or private method works not as intended via the typical black box testing.


Edited by matt77hias, 21 March 2017 - 02:40 AM.