back to article 'I bet Russian hackers weren't expecting their target to suck so epically hard as this'

Welcome back to Line Break, our weekly column of terrible code our readers have encountered in the wild. So far we've featured astonishingly brain-dead designs in production and amusing code from yesteryear machines. Our emphasis has been on learning through others' mistakes while also brightening drab Wednesday mornings with …

  1. Destroy All Monsters Silver badge
    Facepalm

    RIGHT!

    "Many engineers I know, me included, have fallen foul of counting backwards down to zero using an unsigned variable instead of a signed one,"

    Many "engineers" are not using static analysis and/or are switching compiler warnings off while they get a cuppa coffee.

    1. imanidiot Silver badge

      Re: RIGHT!

      I'm not sure how old that particular brainfart is and I'm also not sure all compilers out there have always warned for these kinds of errors. The code is "sort of" valid so a simpler compiler might not notice that particular error

    2. Jimmy2Cows Silver badge

      Re: RIGHT!

      Brain-dead manager: I want all those infinite loop warnings sorted!

      Lazy-arse dev: OK boss...

      [mutters a string of expletives]

      #pragma warning (disable: 4127)

      [heads back to the pub]

    3. Dan 55 Silver badge

      Re: RIGHT!

      No company I have ever worked in has ever opened their wallets for a static code analysis tool no matter how much pleading the programmers have done. Maybe some day this mentality will change...

      1. John Sanders
        Facepalm

        Re: RIGHT!

        ""static code analysis tool""

        Or any other tool.

        Except when the ignorant boss himself likes the tool for some reason :-(

        I had to implement once the client side of the FTP protocol because my boss at the time refused to buy an off the shelf .dll for £100. The pain... the pain... the only positive is that after I finished could troubleshoot FTP issues in the company's network way faster than the network guys.

        In a different company I had to re-write a plotter controller (the 90's children!) to work over the serial port because some ass-hole refused to buy parallel cables or an off the shelf .dll for £50.

        Cases like these is why I do not work as a developer any more, I only code for fun these days.

        1. Alien8n Silver badge

          Re: RIGHT!

          My first foray into asp resulted in the following conversation:

          My boss : "Can we get some software for one of our engineers"

          IT boss : "Why?"

          My boss : "So he can code the department's intranet page, he's writing the pages in asp"

          - to explain, the code was checking a particular folder and creating on the fly an indexed and searchable list of engineering reports. It was quicker than adding the new reports to a static page *every single day*.

          IT boss : "WE HAVEN'T AUTHORISED ANY CODING SOFTWARE! WHAT"S HE WRITING IT IN!"

          My boss : "He wants to know what you're using to write the intranet in"

          Me : "Tell him I'm writing it in Notepad"

          IT boss : "... oh"

    4. Tridac

      Re: RIGHT!

      Coming from an assembler background, where a bad choice of signed or unsigned branch can cause all kinds of subtle bugs, the rule here is never to use a signed type for what is basically an unsigned counting task.

      You can fix the above easily by starting at zero and counting up to a less than the desired limit and it looks far more rational than using a signed type

      unsigned int x = 0;

      for (x = 0; x < limit; x++) {...

      Or, use a do / while to get a similar result, which can produce better asm code, depending on the complier...

      1. Adam 1 Silver badge

        Re: RIGHT!

        @Tridac

        Most of the time you see the count down style, it is of the form

        for (int i=list.Count-1; i>=0; i++)

        {

        if (!list[i].IsStillNeeded)

        {

        list.RemoveAt(i);

        }

        }

        That has its own risk if you attempt with an upwards counting list. Another example is a locate last item in the list matching some criteria.

    5. swm Bronze badge

      Re: RIGHT!

      When I teach C++ I ask if the following loop ever terminates:

      for(int i = 1; i == 0; i++) {

      // do something

      }

      First answer: no - i just keeps increasing.

      Second answer: wait - yes, i wraps around and will eventually come to zero

      Correct answer: who knows - the moment i overflows it is undefined behavior and anything can happen, e.g., the compiler can analyze this loop and notice that i increases from 1 and can never be zero (without undefined behavior) so eliminates the test. It is not well known that integer overflow is undefined and that the optimizer might take full advantage of this.

      Moral - never rely on undefined behavior - this includes type punning.

      Note that the above loop will terminate with unsigned int as the standard says this is guaranteed to wrap.

  2. Anonymous Coward
    Anonymous Coward

    Just use known patterns ffs

    for (uint pos = size; pos-- > 0; ) { /*...*/ }

    1. mangobrain

      Re: Just use known patterns ffs

      Clever as this is, don't play golf with production code. Down-counting loops give me enough pause for thought already, without conflating the test with the counter manipulation, and relying specifically on post-decrement.

    2. AndrueC Silver badge
      Happy

      Re: Just use known patterns ffs

      I'm always a bit leary of embedding '++' and '--' inside expressions. It stems from many (many, many) years ago when one of the versions of Turbo C++ had a bug. As I remember we had a loop that processed an array and the array subscription was incremented during element access.

      The bug was that the subscript indexer inside the loop was only incremented once regardless of how many times the containing for() iterated. I think it might have been related to compiler optimisations so only showed up in release code as well.

      Anyway for me despite the passing years and a move from C++ to C# I still always treat ++ and -- like they were procedures rather than operators.

      1. Vic

        Re: Just use known patterns ffs

        The bug was that the subscript indexer inside the loop was only incremented once regardless of how many times the containing for() iterated. I think it might have been related to compiler optimisations so only showed up in release code as well.

        That sounds like a compiler bug; however iffy the code fragment in question, the middle part of the for loop is supposed to be executed on every iteration...

        But increment/decrement operators become fun when you start using macros. Consider the following :-

        #include <stdio.h>

        #define MIN(a,b) ((a) < (b) ? (a) : (b))

        void main (void)

        {

        int foo = 3;

        int bar = 4;

        int smaller = MIN(++foo, ++bar);

        printf("Smaller value is %d\n", smaller);

        }

        [Apologies for the formatting - ElReg does strange and wonderful things to posts...]

        Should print 4, right? It doesn't.

        I deliberately used pre-increment here to force the error; post-increment is even more dangerous, because altohugh it would print the value you might nominally expect, it doesn't leave that value in foo...

        Yes, this fragement is entirely contrived to demonstrate the point - but I've seen it in the wild.

        Vic.

  3. channel extended
    Headmaster

    One Big Warning!!

    I typically rewrite the code so NO warnings from the compiler occur when doing personal code, for work the push is 'Git R Done' so warnings are ignored.

    If I'm feeling bored, I set pedantic.

  4. hugo tyson
    Mushroom

    Unsigned....

    So what's wrong with

    for ( u = 100; u < 200; u-- ) { ... }

    I think I have seen this as a fix! :-(

    1. herman Silver badge

      Re: Unsigned....

      Man, that 'U<200' almost made me snort my Coke. Brilliant.

    2. Tridac

      Re: Unsigned....

      Would help to have a u++, rather than u-- :-). Also, such counters are often used to index arrays, which start at 0...

      1. Jimmy2Cows Silver badge

        Re: Unsigned....

        Would help to have a u++, rather than u-- :-). Also, such counters are often used to index arrays, which start at 0...

        WHOOSH!

        Assuming "u" is unsigned, I believe you may have missed the rapier-like subtlety of decrementing an unsigned integer below zero...

        That u < 200 will actually work. Sure, there will be 101 iterations of the loop before it exits. And it looks shit. And will confuse anyone reading it. But logically it will still work. Unless "u" is signed, then it's a much longer count.

        Doesn't make it suck less hard though...

        1. Anonymous Coward
          Anonymous Coward

          Re: Unsigned....

          Um. The loop will exit immediately, as u is already < 200. Unless u is a 7 or fewer bit variable, or a signed byte (although even there most compilers would treat 200 as an unsigned byte or signed word, and cast u as an equivalent to enable the comparison).

          Even then, I can't work out why you think there'd be 101 iterations.

          1. Bc1609

            Re: Unsigned....

            What's the IT version of Poe's law? The loop will process while the condition (u<200) is true. If u is unsigned, it will loop round to its maximum value when it passes below zero. This is amusing because it takes the 'bug' in the version of the loop described in the article and turns it into a 'feature'.

            thatsTheJoke.jpg

          2. Jimmy2Cows Silver badge
            Stop

            Re: Unsigned....

            @AC: Sigh... seriously?

            Wow! Am I glad you don't work for me.

            No... it won't exit immediately.

            "u < 200" is not the condition to exit the loop, it's the condition to remain in the loop.

            Therefore the loop will count down quite happily with this condition.

            If "u" were signed, the loop would continue until "u" reaches it's minimum negative value, which depends on the number of bits representing the integer. "u" will then wrap to the maximum positive value for that signed integer type, and the loop will end.

            Assuming unsigned, since the loop begins at 100, it will reach zero before wrapping back to the maximum for an unsigned integer. Which again depends on the number of bits representing it. And is irrelevant, because for anything more than 7 bits, it will be greater than 200 so the loop ends.

            100 to zero inclusive is 101 iterations.

            And do you really want to get into it over implicit vs. explicit declaration?

            Casting i.e. implicit conversion will depend on whether or not "u" is declared explicit, as well as any qualifiers assigned to the constants. In this case, no mention of explicit means we can assume non-explicit declaration. And since the "200" is not qualified (e.g. not "200L"), the compiler should treat it as the same type as "u".

            If not, there will be a warning about signed/unsigned conversion leading to a possible loss of data (unless the warning is disabled, or it's severity level is below the compiler's warning threshold).

        2. Tridac

          Re: Unsigned....

          Sigh. No, it won't have 101 iterations. It will keep counting down, past the initial 100 on it's way, until it wraps round. If it's a (signed) char, 8 bits, it will wrap forever, since the signed value, including the wrap around value, is always less than 200. Similar traps for the careless if you specify 16 or 32 bit int or long, but who in their right mind would code rubbish such as that ?...

          1. Jimmy2Cows Silver badge

            Re: Unsigned....

            @Tridac

            Here's my take on this. Happy to be corrected, but I'm pretty sure it's accurate.

            Assumptions made about the loop code:

            • "u" is an unsigned integer;
            • "u" is at least 8 bits;
            • overflow exceptions won't occur as it's integer arithmetic.

            So to the loop itself: it's counting down from 100, not 200.

            So 100, 99, 98, ... 2, 1, 0

            Then it wraps. And if it's unsigned and anything bigger than a 7-bit integer, the loop ends.

            Thus 101 iterations.

            If the loop counter's signed, well yeah, all bets are off as behaviour depends on the number of bits. A signed char will go to -128 and loop forever as it wraps to +127. Unless there's an overflow exception (which probably doesn't happen in integers - depends on whether or not your compiler can even detect the overflow condition, which in turn depends on whether or not the CPU has integer overflow flags).

            Since the discussion was about unsigned it's reasonable to assume that's the type being used, and so even an 8-bit unsigned char would still let that loop terminate. Again, unless there's an unlikely underflow exception...

  5. This post has been deleted by its author

    1. This post has been deleted by its author

  6. kryptonaut

    Argh

    Why not just simply check for omega == null?

    Because that is not what the function currently does. That is what the comment says it does, which is not the same thing.

    What if alpha and omega are both null? What if alpha is "x" and omega is " " ?

    If you simply replace the code with something that does what the comment says, or what the function name implies, you risk breaking things by changing functionality.

    What if the code is performing correctly (despite being poorly written) and the comment and function name are actually the things that need to change?

    1. Charlie Clark Silver badge

      Re: Argh

      Yep, tests would sort out the confusion. Tests are the easiest way to document your code.

    2. Paul Crawford Silver badge

      Re: Argh

      You have identified two problems with the example:

      1) The comments / "documentation" is misleading, that is not what it is testing!

      2) The code is a convoluted way of trying to express what (I believe) is being tested.

      Better would be something an is_data_null() test to see if pointer is null or empty or 'blank' string, then to return if alpha fails this "null" test but omega passes it.

    3. FatGerman

      Re: Argh

      It looks like a classic example of a function that originally did what the comments say it does but has then been expanded to handle extra cases found during testing/support. Nobody ever updates comments.

  7. RIBrsiq
    Facepalm

    Don't get a VB programmer to port Delphi to C++, got it!

    1. Yag
      Trollface

      You can stop at "Don't get a VB programmer".

  8. DropBear Silver badge
    WTF?

    "Why all this 27 lines, when all you need is 11 characters?"

    Not trying to defend the other errors in that piece of code (especially the sloppy boolean/conditional operator usage) but I'm pretty sure that function was meant to check more that simply just "omega is null" even if that remains undocumented - implying that the "11 character" solution would have been at least equally bugged as the original and the righteous indignation is unwarranted.

    "Skip the second pint at lunch and implement it, ta."

    BLASPHEMY! BURN THE HERETIC! By all means, fix that timing - but lunchtime is sacrosanct and off limits. If it cannot be fixed during work hours, then by Jove, unfixed it shall bloody well stay!

  9. SVV Silver badge

    OmegaIsNull

    I think you missed the true horror of this one by asking why they wrote it instead of simply using if omega == null

    The programmer has chosen to redefine the concept of null to also include non null variables with no value yet assigned, or with a value of a single space. Therefore readers of code calling this function will not be aware of the horrendous side effects that may arise by calling the function if (as they inevitably will) they assume it only checks for actual null values.

    Thiis sort of thing is usually the result of over-caffeinated meetings where a group of developers trying to solve a problem resulting from stupid design decisions made by high and mighty architects (e.g. a space means the same as a null value in this column) end up abusing the language rather than coding with big warning sign comments and naming things well to make sure other developers know this is a horrible cludge.

    1. SolidSquid

      Re: OmegaIsNull

      I'd agree that the omega == null example wouldn't work, but given that the boolean value is already false, is it really necessary to include the alpha == null condition? I'm pretty sure it could be simplified a lot, even if it's not in the way they suggested in the article

      1. #define INFINITY -1 Bronze badge

        Re: OmegaIsNull

        Yeah of course - I think SVV was pointing out that the error the author made is quite a crucial one. Do you understand the effect of the routine rather than its stated intention?

    2. Nigel 11

      Re: OmegaIsNull

      The programmer has chosen to redefine the concept of null to also include non null variables with no value yet assigned, or with a value of a single space.

      Under the (unlikely) assumption that the code does precisely what is required, then its worst feature by far is its name. Call it OmegaIsNullish or OmegaIsEmpty or even YukkyTest. Badly chosen names are one of my pet hates, especially when they've found their way into widely-used libraries or systems from where it's impossible ever to evict them. "select" to wait for IO completion, I ask you!

      Also, comments matter as much as code. Again if the comment said "is Nullish (null or empty string or space)" ...

      But I actually think this is a prize specimen from a person who can't code. There are people who are tone deaf, people who are number blind, and people who can't code. They imitate the forms like any good cargo cultist does, and simply cannot understand that they should find a different employment before somebody fires them (or murders them).

    3. d3rrial
      Paris Hilton

      Re: OmegaIsNull

      Didn't C# have a method named "IsNullOrWhiteSpace()"?

      1. Anonymous Coward
        Anonymous Coward

        Re: OmegaIsNull

        Didn't C# have a method named "IsNullOrWhiteSpace()"?

        Well, from .Net framework 4.0 onwards.

        And it's great to prevent problems and idiot-proofing.

        public bool DoSomethingWithFile(string sFile) {

        bool bOk = false;

        if (!String.IsNullOrWhiteSpace(sFile))

        if (IO.File.Exists (sFile))

        {

        ........

        }

        return bOk;

        }

  10. TeeCee Gold badge

    Yes, but....

    Why not just simply check for omega == null?

    Because it looks to me like what they were trying to do was check for a set of conditions considered equivalent to null rather than just null. i.e. a string with sod-all of consequence in it rather than set to null values, hence the jiggery-pokery with the quoted spaces.

    My guess here would be that the set of values comes from somewhere else that isn't too clever at nulling strings of whitespace......maybe written by the same person.....?

    1. Nick Ryan Silver badge

      Re: Yes, but....

      My guess here would be that the set of values comes from somewhere else that isn't too clever at nulling strings of whitespace......maybe written by the same person.....?

      That'll be anything that has any form of implementation descending from Microsoft Access (which includes the horrible mess of ODBC) where depending on various arcane client settings (as in as a developer you couldn't really guarantee the settings) you may receive a NULL, an empty string or a string consisting of a single space when expecting a NULL.

      1. BongoJoe

        Re: Yes, but....

        That'll be anything that has any form of implementation descending from Microsoft Access (which includes the horrible mess of ODBC) where depending on various arcane client settings (as in as a developer you couldn't really guarantee the settings) you may receive a NULL, an empty string or a string consisting of a single space when expecting a NULL.

        What I would do in such a case is to add the result from the recordset to a zero length string and then shove the thing in a Trim$() statement.

        stringVariable = Trim$("" & oRS("Field"))

        This then would ensure that even if a NULL were encountered it would be added to a zero-length string and nothing would blow up. And the Trim$() would sort out any stray spaces which may have come from $DIETYknowsWhere.

        Nothing wrong with Access, per se. It's just being aware of the pitfalls.

        1. Nick Ryan Silver badge

          Re: Yes, but....

          Interesting trick (always handy), but it does depend on the language and the handling of actual NULL values. In poxy MS T-SQL it'll wind up as a nasty combination of RTRIM(), LTRIM() and NULLIF() which always leaves a horrible taste in the mouth. I could take the combination of NULLIF() and a TRIM() but the lack of a proper TRIM() (i.e. both start and end of a string) always feels bloody annoying. Almost purposefully so. But then n/var/char values are the bastard end of useful and performant in MS-SQL anyway... best compared to glaciers.

          As for Access, I guess so - past a certain version when MS pillaged the FoxPro database format, before which MS-Access was a total train wreck of a mock database, it became at the very least stable and useful for small projects. However in most instances rather than leaving (end) users to implement a database in MS-Access I've always found it considerably more efficient to build, or have built, a proper database system even if it's still in MS-Access but at the very least where the data and interface are separated into different files but usually in something else, again the lowest form of usefulness being an MS-Access front end to an MS-SQL database. On the other hand, it's considerably better than shared spreadsheets that have been bastardised into pretending that they're databases. These still underpin far too many companies and as much as I hate to promote it, even MS-SharePoint is better than this situation. However it's worth bearing in mind that the psychiatrist fees that substantially add to the overall cost of any MS-SharePoint development project should be compared to the legal costs defending assault and battery charges typically incurred when "training" users not to use shared MS-Excel "databases".

          1. Doctor Syntax Silver badge

            Re: Yes, but....

            "On the other hand, it's considerably better than shared spreadsheets that have been bastardised into pretending that they're databases."

            You're probably going to have problems with users who know that Excel is a database.

  11. Dale 3
    Headmaster

    Different languages do bitwise operations differently

    Also, || is a logical operation, | is a bitwise operation. So in the given example x = x || (2^3) not only is the ^ not doing what they expected, but the logical || is also wrong. (Although that probably wasn't the point of the example.)

  12. Phil O'Sophical Silver badge

    endianness

    No-one's mentioned the horrors of having to port between different endian architectures yet.

    Like the network code that works fine on big-endian like SPARC or M68K, but fails on intel, because the programmer had never head of htonl() and friends. Or worse, had heard of them, but forgot one in a critical place.

    Or the weird things that happen when someone has carelessly passed a 32-bit address to a library that expects 16, which works as expected on little-endian systems, but not on big-endian ones.

    Brings back memories of hours of not-so happy debugging

    1. #define INFINITY -1 Bronze badge

      Re: endianness

      you lucky bastard for working on systems with correct endianness.

      1. Peter Gathercole Silver badge

        Re: endianness @#define

        And despite being wrong-endian, Intel x86 is the dominant ISA.

        Even IBM Power has been changed at Power 8 to allow it to work with the same endianess as x86. A retrograde step, IMHO.

        (x86 should have been strangled at birth, not encouraged by IBM!)

        1. Nigel 11

          Re: endianness @#define

          Intel i32, x64_64 is little-endian. So were PDP11 and VAX. Not "wrong"-endian.

          I know it's an issue like the Lilliputians' eggs, but little-endian is surely the logical way. Put a 64-bit integer at X. Load the byte from X+1. You have the bits representing 2**8 up to 2**15 of that integer (8 being the number of bits in a byte). Extending this, you can search an allocation bitmap, define a bit address as 8 times the byte offset of the located bit plus the bit offset within the byte, and go straight to an identified logical block in some storage device at a logical block address matching the logical bit address ( = 8 times byte offset plus bit offset within byte). VAX could do that in one beautiful instruction (back when people still sometimes coded in assembly language)

          Having less-significant bits at higher byte addresses is illogical. If only we wrote right-to-left like some other cultures do, or if pre-computer mathematical convention was to put the least significant digit of an integer first so we'd carry left-to-right just as we write left-to-right, there would be nobody arguing for big-endian, and probably no big-endian hardware at all.

          1. Gordon 11

            Re: endianness @#define

            Having less-significant bits at higher byte addresses is illogical.
            Given that you should be looking at a 2-, 4- or 8-byte number as a single entity there should be no such thinking as "higher byte address". Each number has one address/location.

          2. Dan 55 Silver badge

            Re: endianness @#define

            It's entirely logical. Rotate the bits on a big-endian system then do it on a little-endian system. See the difference?

          3. Nick Ryan Silver badge

            Re: endianness @#define

            Somehow or other I'd never actually noticed the disparity in L-R languages compared to the (arabic?) number layout system (the common alternative Roman numbering system available at the time [AFAIK] was just a work of art of almost purposeful obfuscation with an intention to be useless). It would genuinely make more sense if one hundred and twenty three were represented 321 (and read as three twenty hundred).

    2. Vic

      Re: endianness

      No-one's mentioned the horrors of having to port between different endian architectures yet.

      I worked on MediaHighway for a couple of years.

      I don't know if it changed after I left[1], but back then, the ABI was specified, rather than the API. So everything was big-endian as it traversed the interface.

      Thus, on a little-endian system, the app code spent the lower layer byte-swapping everything into big-endian, and the top layer of the MH chunk spent its time byte-swapping back into little-endian. Ho hum...

      Vic.

      [1] I did ask them to change this, but it hadn't happened by the time I left.

    3. swm Bronze badge
      Stop

      Re: endianness

      The ether net specification sends bits least-significant bit first. The alto computer ordered 16-bit words most-significant byte first. Double words (32-bits) were low 16-bits followed by high 16-bits. Pop quiz: which bit of a double word is sent first? (I didn't believe this mess could possibly be true until I looked at the wiring diagrams.)

      Answer - some bit in the middle (are we counting from the left or right end of the double word and are we counting from zero or one?)

      No, it has been too many decades for me to sort this out now.

    4. CanadianMacFan

      Re: endianness

      How about worrying about that and tossing converting everything from 6-bit to 8-bit into the process? I was part of a group that was moving an application over from a batch process to a on demand so that it would run on the web. It was a program with a very old history and had been ported a number of times. It started out on a 6-bit mainframe. However because it was for doing name searches when you start a business they were very cautious in making changes. So when they ported it to a new architecture they just made sure that it ran but never changed the program or data structure. There were a lot of small programs reading and writing temporary files, in 6-bit format, running on whatever the latest servers they were using.

      It wasn't so bad because it was a nightly batch process. However once you start running this on demand all of this converting between little and big endian and 6-bit and 8-bit for every process really started to slow things down. We were able to show them what sort of improvement they could expect by removing the conversion and just storing their temporary data in 8-bit but they didn't go with it because it was close to the release and it would have involved much more testing than the changes we had already made. Of course the alternative would have been to rebuild the application so that it combined all of the many applications into one and that would have saved the writing of the temporary data and all of the process creation too.

  13. This post has been deleted by its author

  14. AbelSoul
    Pirate

    Re: don't write PHP like the above

    I'll sheepishly raise my hand and admit that, while still at college, my first attempt at a site used PHP like that (un-parameterised queries).

    I don't think anyone here will be surprised to learn that the site was subsequently hacked.

  15. Charlie Clark Silver badge
    Facepalm

    PHP always makes me cringe

    "select MAIN_TABLE.`product_id` from `{$tableName}` as MAIN_TABLE where MAIN_TABLE.`request_path` in('{$path}')

    So much not to like.

    Why is the table name being parametrised? That's a recipe for disaster all in itself. But, FFS, any field in the DB that depends upon part of the request is suspect.

    The SQL injection vector could be mitigated against in the method of the object returned by getDatabaseConnection() which could even check for incorrect use.

    1. Anonymous Coward
      Anonymous Coward

      Re: PHP always makes me cringe

      "Why is the table name being parametrised?"

      Because someone was taught to do that at U and has never questioned it ever since.

      I was once confronted with a class with a fairly simple functionality that required a large xml resource file. A number of tables had all been parameterised and the resource file converted one table name to another. When I asked why I was told, "Because the table names in the schema might change, and then without the xml file things would stop working"

      So...what happens when someone somehow changes the schema, and someone forgets to fix the xml file? Exactly the same as not reviewing the source code, that's what. But xml is magic fairy dust.

      And if you are altering the schema often enough for that to be an issue, you have other problems.

      1. Brewster's Angle Grinder Silver badge

        Re: PHP always makes me cringe

        I hate XML, but have you ever had to patch an executable to change a hard coded parameter?

        1. Anonymous Coward
          Anonymous Coward

          Re: PHP always makes me cringe

          "I hate XML, but have you ever had to patch an executable to change a hard coded parameter?"

          That's a fair point and in some environments xml is the answer. Our environment was rather different; configuration data was stored in a [local] Cloudscape database, and every new version of the executable had a first load routine to make any necessary changes to the configuration. Updates therefore required a lockstep of (a) DBA runs any schema changes via an update script and (b) IT install software update to take advantages of schema changes. Adding xml was a pointless complication.

        2. Wensleydale Cheese
          Unhappy

          Re: PHP always makes me cringe

          "I hate XML, but have you ever had to patch an executable to change a hard coded parameter?"

          Back in the day, patching an executable was a far better option than having to deal with the folks who mangled the tape conversions involved in getting a new executable to the customer.

      2. Killer Squid

        Re: PHP always makes me cringe

        Can be useful where various sites using the same software may use a site-specific view in place of the Main Table. The XML would be site specific, but the code common to all sites....

        1. Charlie Clark Silver badge

          Re: PHP always makes me cringe

          Can be useful where various sites using the same software may use a site-specific view in place of the Main Table. The XML would be site specific, but the code common to all sites....

          This a bit short of context: the DB would be doing something with XML? How so?

          In any case I still can't the need for the client code to parametrise the table name like this: that really ought to be done by the DB.

      3. Doctor Syntax Silver badge

        Re: PHP always makes me cringe

        "And if you are altering the schema often enough for that to be an issue, you have other problems."

        This is the problem with the code first, design* later approach.

        However you should have taken whoever told you that to one side & explained views - although it sounds like the sort of shop where that approach could lead to a tangle all on its own.

        *Design? You should be so lucky.

  16. Robin

    Almost

    the buggy code below was almost exploited by Russian hackers, we're told.

    I almost won the lottery last week, but I didn't.

    Seriously, it would have been interesting to know how it was almost exploited.

    1. Lysenko

      Presumably...

      ...error logs full of faulty parameters that couldn't quite match the syntax needed to trigger "full retard" but came damn close.

    2. Anonymous Coward
      Anonymous Coward

      Re: Almost

      As the article says, there were hundreds of attempts, but none of them accounted for the fact that the parameter is in parentheses. Failing to include a closing parenthesis meant the attempted exploits didn't result in well-formed SQL. If only they'd thought to copy their SQL injection code from XKCD: https://xkcd.com/327/

    3. Nigel 11

      Re: Almost

      Seriously, it would have been interesting to know how it was almost exploited.

      All you really need to know is here

    4. DanielsLateToTheParty
      Boffin

      Re: Almost

      The story in the article is mine, it happened to me last week. As Lysenko realised there were errors generated by badly formed SQL from requests with " '; " (hundreds as mentioned in the article) but none with " '); ". Serious errors like that notify me directly rather than waiting in a forgotten log file forever.

      After dealing with that and patching in a hurry, I went back and grepped for the attacking IP address and found over 65000 requests. Most seemed to be completely benign. From using pen-test tools I know that the first stage is to spider a site and that generates the most traffic. Some attacks contained PostgreSQL or MS SQL specific functions which suggests they didn't know it was a MySQL site. So it looks like a mostly automated attack from a single address in a Russian IP block.

      The forensic aspect is fascinating. Kind of like CSI only real. I'd love Register to do an autopsy of a more complicated attack some time.

      1. allthecoolshortnamesweretaken

        Re: Almost

        "The forensic aspect is fascinating. Kind of like CSI only real. I'd love Register to do an autopsy of a more complicated attack some time."

        Yes please!

      2. Robin

        Re: Almost

        The forensic aspect is fascinating.

        Agreed! That's what I was getting at. Cheers for the extra info :-) And yes an autopsy of a full-on attack would be interesting.

        Glad you escaped disaster!

  17. ZanzibarRastapopulous

    The PHP one.

    Perl has the concept of tainted data, does this exist in PHP?

    1. Dan 55 Silver badge
      Trollface

      Re: The PHP one.

      PHP has the concept of tainted code.

  18. Novex

    Re the PHP, I prefer not to even construct any SQL on a client if I can possibly do so. I find it much better to have only stored procedures as the visible interface of a database server, meaning no internal structures of the database are visible to the outside world. Also, that way I can write the SPs to have well-formed SQL checking that no external system can even hope to get round.

    Re the final example with the unsigned integer, and bear with me as I'm a bit of a noob in C or C++ territory, but checking for 'i > 0' or 'i >= 1' as the condition in the for loop, rather than 'i>=0', would work wouldn't it?

    1. Charlie Clark Silver badge

      There's a lot to be said for insulating the DB but SPs bring their own problems, not least having a different code base.

      As long as parameters are being passed correctly there's not much to be said against giving the client some access. If you don't you're likely to find client code filling up with stuff better done on the db where developers either don't know or don't know how to do it on the server.

      The best approach I've seen here is the one suggested by Hannu Krosing of keeping the code on the client side but effectively shipping it to the DB to run there. YMMV but I think there's a lot to be said for this.

      1. Dan 55 Silver badge

        An argument for using SPs and only SPs is if you allow query passing and your website does get owned, the database can be queried with anything.

        1. Novex

          An argument for using SPs and only SPs is if you allow query passing and your website does get owned, the database can be queried with anything.

          I was going to say something similar. I always work on the principle that 'the client can never be trusted'. In the case of a database, a client is anything connecting to the database, not just a traditional client machine. So a web server would be a client to the DB, and if the web server got owned then the SQL could all be rewritten. It would be even better if there was another authorisation & authentication layer between web server and DB on a different virtual or physical machine that held the DB connection information. The chances of such a mid-tier being owned as well as the front end / web server is even less then.

      2. ZanzibarRastapopulous

        > ...but SPs bring their own problems, not least having a different code base.

        Even just a shim layer over the database would allow the database code to be reviewed and of course makes it much easier to convert if you change databases.

        Making db calls from all over your code is definitely the wrong approach.

        1. Charlie Clark Silver badge

          Even just a shim layer over the database would allow the database code to be reviewed and of course makes it much easier to convert if you change databases.

          Making db calls from all over your code is definitely the wrong approach.

          Absolutely. As regards changing the DB, I'm not sure how much of a real issue this is. But it shouldn't be held back by DB specific calls in the application layer.

    2. Brewster's Angle Grinder Silver badge

      "but checking for 'i > 0' or 'i >= 1' as the condition in the for loop, rather than 'i>=0', would work wouldn't it?"

      Yes.

      1. Simon Harris Silver badge

        "but checking for 'i > 0' or 'i >= 1' as the condition in the for loop, rather than 'i>=0', would work wouldn't it?"

        That depends on whether you need to do anything within the body of the loop when i is 0.

        I've been caught out a few times with the unsigned int, typically...

        std::vector< sometype> a_vector;

        ...

        int length = a_vector.size();

        for ( int i = 0; i < length; i++ )

        {

        do something with a_vector[i]; // Yes, I know an iterator would probably work too in this case.

        }

        This will typically give a compiler warning as the size type is unsigned (although it will work counting up or counting down), so in an attempt to remove compiler warnings, the alternative:

        std::vector< sometype> a_vector;

        ...

        unsigned int length = a_vector.size();

        for ( unsigned int i = 0; i < length; i++ )

        {

        do something with a_vector[i];

        }

        will compile without the unsigned -> signed conversion warning.

        Being unsigned normally works, and it becomes automatic to use it, specially if your code is counting up most of the time, until you have a situation where you have to work from the end of the vector towards the beginning, and forget about the wrap-around on:

        for ( unsigned int i = length-1; i >=0; i-- )

        1. sed gawk Silver badge

          Dude: this is just wrong

          1) std::vector<T>::size_type is never spelt int

          2) Use iterators

          // C++ 03

          #include <vector>

          #include <iostream>

          // C++ 03 access elements from vec[ vec.size() -1 ] to vec[ 0]

          std::vector<int> a_vector;

          std::vector<int>::const_iterator end(a_vec.rend());

          for(std::vector<int>::const_iterator beg(a_vec.rbegin()); beg != end; ++beg){

          std::cout << "An integer value: " << *beg << "\n";

          }

          // C++ 03 access elements from vec[ 0 ] to vec[ vec.size() -1 ]

          std::vector<int> a_vector;

          std::vector<int>::const_iterator end(a_vec.end());

          for(std::vector<int>::const_iterator beg(a_vec.begin()); beg != end; ++beg){

          std::cout << "An integer value: " << *beg << "\n";

          }

          3) Use C++11

          // C++ 11 access elements from vec[ 0 ] to vec[ vec.size() -1 ]

          std::vector<int> a_vector;

          for(const auto & el : a_vec){

          std::cout << "An integer value: " << el << "\n";

          }

          // C++ 11 access elements from vec[ vec.size() -1 ] to vec[ 0]

          std::vector<int> a_vector;

          const auto end = {a_vec.rend()};

          for(const auto beg = a_vec.rbegin(); beg != end; ++beg){

          std::cout << "An integer value: " << *beg << "\n";

          }

          1. Richard 12 Silver badge

            Re: Dude: this is just wrong

            Iterators are slightly slower and usually harder to read.

            The former usually doesn't matter, the latter always does.

            Readability trumps most things. Be nice to You-from-the-future.

            They probably think you're an idiot, but hopefully you can make sure they don't think you're malicious.

    3. streaky Silver badge

      I prefer not to even construct any SQL on a client if I can possibly do so. I find it much better to have only stored procedures as the visible interface of a database server, meaning no internal structures of the database are visible to the outside world

      I'm usually happier when rdbms don't support stored procedures at all - not for nothing but what you're saying for most software is all sorts of doing it wrong for a list of reasons it'd take way too long to list.

      Just to be clear nothing happening here is the fault of PHP. With only minimal competence the average 8 year old should be capable of writing code that's impossible to SQL inject. The end.

  19. Androgynous Cupboard Silver badge

    To encode these bits, the engineer decided to bitwise OR the destination byte with 2 raised to a particular power representing the bit position. "Bad approach, I know"

    It's only a bad approach because there is no "to the power of" operator in C. Otherwise it's fine I think - I would expect a decent compiler in any language would treat "2**3" and "1<<3" as identical (when 2 was a numerical constant)

  20. Simon Harris Silver badge

    Here's a signed/unsigned type conversion that got me the other day.

    I was writing something to read a set of X,Y points and, with a 1 unit resolution in the X direction record a maximum height map of the points in an array. I knew the X values were limited to the range -25 to 99, and all the Y values were positive so I did (simplifying things a bit here):

    double heightmap[125];

    for ( unsigned int i = 0; i < 125; i++ )

    {

    heightmap[i] = 0;

    double xpos = i - 25;

    unsigned int listsize = list.size();

    for ( unsigned int j = 0; j < listsize; j++ )

    {

    if ( abs( xpos - list[j].X ) < 0.5 )

    {

    if ( list[j].Y > heightmap[i] ) heightmap[i] = list[j].Y;

    }

    }

    }

    (for the sake of brevity, the boundary case of an X value being exactly something.5 have been ignored)

    It took me a while to work out why I was getting no data in entries 0..24.

    Of course, because i was unsigned, for entries 0..24 xpos was actually a very large positive number instead of -25 to -1, the type conversion was happening after trying to subtract 25.

    Moral - always remember at what point in a calculation types are converted, and if it's not obvious (or where you want them) force the conversion yourself.

    (Mods, is there any way to keep the indentations in code snippets?)

    1. Stoneshop Silver badge

      Re: Here's a signed/unsigned type conversion that got me the other day.

      (Mods, is there any way to keep the indentations in code snippets?)

      < code > and </ code > should do that, but the spaces (or tabs) still get eaten by the Comment Space Eater:

      double heightmap[125];

      for ( unsigned int i = 0; i < 125; i++ )

      {

      heightmap[i] = 0;

      double xpos = i - 25;

      unsigned int listsize = list.size();

      for ( unsigned int j = 0; j < listsize; j++ )

      {

      if ( abs( xpos - list[j].X ) < 0.5 )

      {

      if ( list[j].Y > heightmap[i] ) heightmap[i] = list[j].Y;

      }

      }

      }

    2. Brewster's Angle Grinder Silver badge

      Yeah, you've got to know the integer promotion rules.

      Incidentally, listsize should be size_t (old money) or the something equivalent to decltype(list)::size_type (new money). But I'm a javascript programmer, so what do I know.

  21. Ken Hagan Gold badge

    Nicely deadpan

    "You can probably guess what he did to fix it."

    Lemme see, he measured the speed of his current PC and *worked out the number that would now be required ... to *several significant figures* because, well, he's a pro who takes pride in his work as opposed to the original numpty who just picked a round power of ten.

    1. Nolveys Silver badge
      Headmaster

      Re: Nicely deadpan

      Lemme see, he measured the speed of his current PC and *worked out the number that would now be required ...

      import_time

      def_myDelay(seconds):

      ________start=time.time()

      ________count=0

      ________while_time.time()-start<1:

      ________________count+=1

      ________for_i_in_range(seconds-1):

      ________________for_j_in_range(count):

      ________________________pass

      def_getTomorrowsDate():

      ________myDelay(24*60*60)

      ________return_time.localtime()

  22. FatGerman

    Timer not running

    I recall finding a bug in a proprietary mobile OS emulator, in the days when there was more than just iOS and Android*

    The OS used POSIX signalling. A lot. Timers were a very important part of this. You'd create a timer, which would be given an ID by the OS. And then some time later you'd get a signal saying your timer had expired and which ID it was. Quite neat, quite useful. Except that the timer ID was a 32 bit unsigned number, automatically incremented by the OS every time a timer was created. And the ID where every bit was '1' was used to indicate 'this timer has expired, please clean up the memory associated with it'. But the routine that assigned the IDs was blissfully unaware of this fact, meaning that while running our test suite, every 38 hours or so a timer would be created with an ID that would mean it would never expire. Took me a week of scouring debug logs to twig that one.

    *Yes I know there's Windows Phone, but I'm expecting that to be dead by the time I finish typing.

    1. DJV Silver badge
      Thumb Up

      Re: Timer not running

      [upvoted mainly for the last (excellent) sentence]

  23. Luiz Abdala

    I can create the algorithm, but I can't for the life of me to code properly. My college teacher told me that, and he was right.

    These days I just write the algorithm down, describing what I want the code to do, and let a competent coder to sort it out.

    I was able to code in Pascal (Pre-delphi) though.

    1. sed gawk Silver badge

      I flatly don't belive this to be possible

      I struggle to see how this is possible.

      Perhaps assuming a spherical cow in a vaccum ?

  24. Lee Mulcahy

    XML == cool?

    "Also, there's got to be a better way to store configuration data than using raw binary. JSON, YAML, or (dare I say it) XML are pretty cool instead."

    Yes, let's used bloated, overwrought file formats instead of compact, efficient storage, and open it up so that anyone with a text editor can manipulate it.

  25. Cynic_999 Silver badge

    Binary data is bad ???

    "

    Also, there's got to be a better way to store configuration data than using raw binary. JSON, YAML, or (dare I say it) XML are pretty cool instead.

    "

    For some values of "better" perhaps. Operating on raw binary is usually faster because the CPU can manipulate it directly without converting or parsing. It also takes up far less room, possibly reducing a database to a tenth the size (or less) of the given alternatives, which in turn speeds up searches. Of course these days the accepted standard is to use human-readable databases (not that any human is ever likely to do so), but spend 10 times as much money on hard drive array, RAM and processing power. That way your 3GHz 4-core 64 bit system will perform almost as well as the Z80 4MHz platform the original algorithm was written for.

    1. sed gawk Silver badge

      Re: Binary data is bad ???

      I'd rather serialise and deserialise from JSON over using DER / ASN.1.

      I'd rather write an DER / ASN.1 parser (it's two trivial functions and happy days ) than a JSON parser.

      Figuring out whats wrong with the broken lump of json is painful, figuring out what wrong with the ASN.1 is painful.

      It's as much about tooling as any comment on fitness for purpose

    2. ZanzibarRastapopulous

      Re: Binary data is bad ???

      Binary formats are smaller and faster, but people have to write a tool to manipulate them. In my experience this doesn't always happen and a few years down the line you get left with a binary config no-one can change.

  26. Anonymous Coward
    Anonymous Coward

    The pointless use of a ternary operator when there is no 'else' action

    The article quotes "the pointless use of a ternary operator when there is no 'else' action", which is confusing me somewhat.

    I can only see one ternary in the code, in the line that goes:

    $path[0] == "\/" ? $path = substr($path, 1, strlen($path)) : $path;

    Unless I'm being overly dim, that clearly has an 'else' action at the end of the line. In effect, it is saying if the path begins with a forward slash, remove it, *else* leave it alone.

    I know technically it accomplishes nothing as $path is unchanged but nevertheless there *is* an 'else' action. It looks like the coder is trying to save screen real estate and using the ternary as a shorthand to avoid the syntactic sugar of an 'if' condition.

    Please note that *I am not speaking in favour of this code*, just saying that there is in fact an 'else' action which sort of, in a roundabout way, has some (very poor) possible justification.

    1. Cynical Shopper

      Re: The pointless use of a ternary operator when there is no 'else' action

      I think you may have misread the line as:

      $path = $path[0] == "\/" ? substr($path, 1, strlen($path)) : $path;

      As it is, the code is equivalent to:

      if ($path[0] == "\/")

      ___ $path = substr($path, 1, strlen($path))

      else

      ___ $path;

      I'd equate that to "no else action".

  27. Anonymous Coward
    Anonymous Coward

    The PHP hate

    There is a terrible stigma attached to PHP.

    This is not helped by the daft inconsistent naming conventions, the abandonment of version 6, and the fact that it is so quick and easy for the newbie that it attracts many inexperienced developers with the code issues that inevitably causes.

    The steaming pile of shite that is the Wordpress codebase doesn't help. I abandoned that festering security hole after my blog got hacked through it. I suspect the Linux Mint people may make the same decision.

    I 've been doing C# for over a decade but every now and again I return to - in a technical sense if the wife is reading - my second love PHP (my first was Modula-2). From the mid version 5s onwards the difference is huge in comparison to the early versions.

    It is now possible to write structured, class-based, clean, interfaced, tested, abstracted, secure, fast, readable code in the same way as it's language peers. If you want to see what can be done, consider Laravel for instance.

    I think it is approaching time for developers to think again when it comes to blanket dismissals of PHP. Though I also think it's time for the language's maintainers to try and increase the barrier to entry slightly - just enough to steer the worst amateurs towards Node instead so that the PHP reputation can be rescued.

    Oh, and by the way I know I was one of those amateurs in my own early days so I am not meaning to be insulting there, just honest about the code produced by beginners who are not faced with a particularly steep learning curve.

  28. Stevie Silver badge

    Bah!

    Oi, "Daniel", watch who you are bad-mouthing!

    I can't be the only former Fortran and Cobol programmer who went on to bigger things and now shakes his head at what he sees the Bright Young Things with fresh CS degrees perpetrating, secure in their knowledge that computers were invented in 1986 so what would that old fart know?

  29. BongoJoe
    Mushroom

    Boolean variables not as True or False

    I find it's fun* when someone assumes that one can handle an variable of any type and treat it as a Boolean.

    The trouble is that some languages assume that True is -1 and that others assume that True is non-zero or that False is zero or not-one or...

    Cue arrival of someone who loosely types and is new to the language...

    *another loosely typed definition

  30. Seajay#
    Pint

    "Skip the second pint at lunch"

    Woah woah woah. We all want good code but this is surely going too far.

    1. Adam 1 Silver badge

      And to my mind counter productive.

      https://xkcd.com/323/

  31. Anonymous Coward
    Anonymous Coward

    Importing

    The code was "imported" from an external team, seems like there are some CYA verbal acrobatics going on here - they built the site or you used their code in yours - it is up to you to review it.

  32. Alan Brown Silver badge

    some naming is in order

    "professional company, or I could say cowboy posse,"

    Outfits which do this rely on NOT being named and shamed.

  33. I. Aproveofitspendingonspecificprojects

    What's that sound you make when you come to the end of a long line of The Register tabs and realise you can close down now and concentrate on the bit of apple stuck between your teeth just as you realise you never finished that last article you started scrolling through before moving back to the head of the queue and forgot about it and closed the tab?

POST COMMENT House rules

Not a member of The Register? Create a new account here.

  • Enter your comment

  • Add an icon

Anonymous cowards cannot choose their icon

Biting the hand that feeds IT © 1998–2019