Dilbert





The Daily WTF

CodeSOD: Assert Yourself;

Chris V does compliance testing. This often means they trace through logic in code to ensure that very specific conditions about the code’s behavior and logic are met. This creates unusual situations, where they might have access to specific and relevant pieces of code, but not the entire codebase. If they spot something unusual, but not within the boundaries of their compliance tests, they just pass on by it.

One of the C++ code bases Chris had to go through featured this “defensive” pattern everywhere.

if (someConditionalStatement)  
{
    // do stuff  
}
else  
{
    runtime_assert(false && "some condition was not met");  
}

Chris doesn't have access to runtime_assert's definition. It *is* possible to `&&` a boolean and a string together- you get a boolean result, though. So the `"some condition was not met"` message just vanishes, as if it weren't there. Literally, this is just runtime_assert(false). Which, excepting the extraneous string, almost makes sense, if you want the failed condition to trigger some exception/error pathway. Then there were these variations on the pattern:

if (someConditionalStatement)  
{
    // do stuff  
}
else  
{
    runtime_assert(someConditionalStatement);  
}

Which, again, essentially just means runtime_assert(false). And again, this almost makes sense. As Chris explains:

I mean, it works. It all works. It’s just a weird and unnecessary way to write it. …I get the feeling they have heard about error checking and defensive programming, but haven’t heard about exceptions and exception handling?

[TDWTF Survey Reminder] Take our Developer Mentorship Survey asking developers about their managers, what works, and what doesn't. Help out your fellow devs and to enter to win a TDWTF mug!


CodeSOD: One Way to Solve a Bug;

Startups go through a number of phases, and one specific phase is the transition from "just get it done and worry about the consequences tomorrow" into "wait, maybe if we actually did some planning and put some process around what we do, we won't constantly be one step behind the current disaster."

And that's when they start to hire people who have more management experience, but are also technical enough that they can contribute to the product directly. At BK's company, the latest hire in that category is Sylvester.

Sylvester is the new team lead, and he comes from a more "enterprise" background, which means he's had a very difficult time getting up to speed, and is unwilling or uncomfortable to make decisions with limited information. And also, Sylvester might not be particularly good at the job.

BK noticed that Sylvester had a commit sitting in code review, and it had been sitting there for some time, so they took a look. One of the first things they spotted was a method called SolveBug, which made it clear they were in for a "treat".

void SolveBug (std::vector<BusinessEntity> container){ std::sort(container.begin(), container.end()); const auto it = std::unique(container.begin(), container.end()); if (it != container.end()) { container.erase(it); } }

So, one of the business rules about the vector of BusinessEntity objects is that the entries should be unique. BK skimmed this method, and without looking at it too deeply, and without knowing the definition of std::unique, made some assumptions based on the code. The std::unique function must return the first duplicate, and then we erase it.

That couldn't be right though, because what if there were multiple duplicates? So, BK did what Sylvester obviously didn't, and did a quick search. std::unique returns an iterator where consecutive duplicates are removed.

BK, being generous, assumed they were missing something, and asked Sylvester what that might be.

"Oh, thanks for spotting that! That's probably why it doesn't work."

Apparently, Sylvester had pushed code he knew was broken, and didn't mention it to anyone. He didn't ask anyone to take a look beyond submitting a code review. He didn't bother to check the documentation, and certainly didn't think about the code he was writing.

Perhaps "SolveBug" was less a description of what the method did, and more what Sylvester hoped the code review would do? In that case, the method was very accurately named.

[TDWTF Survey Reminder] Take our Developer Mentorship Survey asking developers about their managers, what works, and what doesn't. Help out your fellow devs and to enter to win a TDWTF mug!


Error'd: Watch the Skies!;

"In light of the imminent UFO strike, I may need to reconsider my flight plans...or leaving my house in general," writes Pedro.

 

Howard wrote, "I guess, in Dell's eyes, it's their site and anything can be on sale if they say it is."

 

"Hmmmm...Do I really trust Google Play Music to manage my Google Play Music library?" Ryan S. writes.

 

Rob K. writes, "I'm guessing I should take advantage of the sale price before Dan gets back to his computer."

 

"Saying that the app isn't working correctly is a bit obvious in this case," writes Jerry.

 

Adrien wrote, "The exception L'opération a réussi (Operation succeeded) being thrown leads me to believe there is nothing wrong here and this application is simply intolerant of the French language."

 

[TDWTF Survey Reminder] Got 5 minutes to tell your manager what you really think? Take the Developer Mentorship Survey and there just might be a free TDWTF mug in it for you.


CodeSOD: Overlapping Complexity;

After his boss left the company, Joel C was promoted to team lead. This meant that Joel was not only responsible for their rather large production codebase, but also for interviewing new potential team members. There are a ton of coding questions that one can ask in a technical interview, and Joel figured he should ask one that they actually solve in their application: given two unordered sets of timestamps, calculate how much overlap (if any) is between the two series.

If you think about it for a minute, it's really quite simple: first, find the minimum and maximum values for each set to get the start and end times (e.g. [01:08:01,01:09:55] and [01:04:11,01:09:42]). Then, subtract the later start time (01:08:01) from the earlier end time (01:09:42) to get the overlap (01:09:42 - 01:08:01 = 00:01:41). A non-positive result would indicate there's no overlap (such as 12:00:04 - 13:11:43), and in that case, it should probably just be zero. Or, in a single line of code:

return max(min(max(a), max(b)) - max(min(a), min(b)), 0)

Of course, something more spaced out might help with readability, but Joel saw a lot of candidates overthink the problem. They would sort the lists, create unneeded temporary variables, not understand that they really only need the first and last elements of the list, etc. In many of those cases, Joel judged candidates quite harshly; it's a simple problem and if this confuses them, how could they handle more complex problems?

A handful of candidates recognized the problem for how simple it was, but one went so far as to ask, "there are a ton of ways to solve this in code; here's my solution, but I'm really curious how you solved it?"

Joel wasn't really sure how his former boss solved the problem. After spelunking through the codebase, he found out:

def compute(dev_a, dev_b):
    labeled_timestamps = []
    for label, dev in ('a', dev_a), ('b', dev_b):
        for t in dev.timestamps:
            labeled_timestamps.append([t, label])
    labeled_timestamps.sort()

    last_label = None
    start_overlap = None
    end_overlap = None
    last_t = None
    for t, label in labeled_timestamps:
        if last_label is not None and last_label != label:
            if start_overlap is None:
                start_overlap = t
            else:
                end_overlap = last_t
        last_label = label
        last_t = t

    if end_overlap is None:
        end_overlap = start_overlap
    overlap_time = pd.Timedelta(end_overlap - start_overlap, 's')

"I have seen some overly complex answers," Joel wrote, "but this is a level of overthinking that is beyond impressive. If someone had given me this in an interview, I would probably have been left completely dumbfounded, and submitted this as a Tales from the Interview. Instead... I just shut down my computer and started my weekend drinking a little early."

[TDWTF Survey Reminder] Don't miss your chance to tell managers what you REALLY think about good (and WTF-worthy) dev mentorship. You might win a TDWTF mug for participating!


The Most Secure Option;

“The auditors have finished examining our codebase.”

That was how Randy’s boss started the meeting, and she delivered the line like a doctor who just got the tests back, and is trying to break the news gently.

After someone in another department did the whole “I found a thumb drive in the parking lot, let me plug it into my work laptop!” thing, management realized that they hadn’t done any kind of security evaluation in years, and brought in a bunch of highly paid consultants to evaluate their practices. Part of that meant doing audits of their software portfolio for compliance with the new security standards.

Now, Randy’s boss was running a cross-functional meeting- developers, operations, and even a few support desk representatives, to review the audit results. Most of the hits they took on the audit were the kind of slipshod stuff that accrues over years of under-budgeted, over-specced projects. Passwords stored in source control. A few SQL injection vulns. But the one that seemed like an easy win was the fact that they didn’t use any SSL on their web applications.

“Oh, we should be able to fix that, easy,” Randy said.

“Oh, we should, should we?” Benny, the sysadmin said. He leaned over the table, with his hands clasped. “How many SSL certs have you provisoned?”

“Well, a bunch, I’ve-”

“Because I have, and it’s no walk in the park, and it’s very expensive.”

Randy blinked, and glanced over at his boss. She didn’t have anything to add.

“That’s… not true?” Randy said. “It’s not that expensive to buy a cert, but we can also go with LetsEncrypt, which is free.”

“Ah ha!” Benny said. “It’s very expensive to do it right. You can’t just use some service from the Internet. We’re here to talk about our security audit, and using LetsEncrypt is not possible. Anything hosted externally and accessible via the Internet poses a huge organizational risk. Free SSL from the Internet is an easy target for a hacker.”

“Right,” Randy’s boss said. “We’ll table this for now, but it looks like we probably won’t add SSL until we have a better sense of the costs.”

“My advice is that we don’t use SSL at all,” Benny said. “That will be more secure than what Randy’s proposing.”

The audit happened early this year. No one has yet formulated a plan to move to SSL.

[TDWTF Survey Reminder] Don't miss your chance to tell managers what you REALLY think about good (and WTF-worthy) dev mentorship. You might win a TDWTF mug for participating!


Representative Line: Time Dilation;

A good variable name is clear and specific about what the variable does. But sometimes you can have a variable name that's perhaps a little too specific. Victoria found this representative line of Rust code:

let threeseconds = time::Duration::from_secs(60);

Time certainly can stretch when you're deep in debugging, and three minutes can feel like an hour. And as you cross that event horizon, you start asking yourself questions. It's easy to understand how this code came to be: they though they needed three seconds for some task, but actually needed 60. They'd already made the variable name, and didn't want to trace through changing it everywhere.

But that's not really an explanation. Victoria shares her questions:

I then started wondering "so how did that code come to be? What kind of problem required them to think they'd need three seconds, but then bump that value up to 60?". That went a bit further with "So, judging by this representative line, what would the rest of the code look like, how would it even work?". I still haven't figured that last part out.

Don't worry, Victoria, if you need more time to work on understanding it, I know a great programming trick to make more time…

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!


CodeSOD: A Botched Escape;

Nancy was recently handed a pile of "modern" PHP that weighs in at tens of thousands of lines of code.

This is how every query is executed:

function getFoo($bar) { $bar = my_escape($bar); $sql = " select * from foo where bar = '" . $bar . "' "; return do_query($sql); }

Yes, this is a SQL injection vulnerability. No, there is no part of the application which uses parameterized queries. But wait, they call my_escape. That must be safely escaping the input so it can be used as a query param safely, right?

function my_escape($data) { if ( !isset($data)) { return ''; } if ( is_numeric($data) ) { return $data; } if(empty(trim($data))) { return ''; } $non_displayables = array( '/%0[0-8bcef]/', // url encoded 00-08, 11, 12, 14, 15 '/%1[0-9a-f]/', // url encoded 16-31 '/[\x00-\x08]/', // 00-08 '/\x0b/', // 11 '/\x0c/', // 12 '/[\x0e-\x1f]/' // 14-31 ); foreach ( $non_displayables as $regex ) { $data = preg_replace( $regex, '', $data ); } $data = str_replace("'", "''", $data ); return $data; }

So, this strips off url encoded characters, as well as hex-values. It, ah, doesn't strip off any of the characters which you might want to strip off if you're trying to protect against SQL injection. These regexes are case sensitive, too, so while %0b will get stripped, %0B won't. Which is also irrelevant, because by the time you're getting to passing things into the database, they've already been percent-decoded.

For extra fun, because the regexes are applied in a loop, it's possible that the regex might remove more than intended, because the first regex might strip characters which results in something that's picked up by a later regex.

For example, "A%%001%0cfB" turns into "A%1fB" after the first pass, then "AB" after the second one. But a slightly different string, like "A%%100%1cfB" turns into "A%0fB".

Now, those are contrived examples, but it's an interesting illustration of poorly thought out code. Looking at this, my guess is that someone knew they needed to sanitize their database inputs, but didn't know exactly how. They saw a my_escape function in the codebase, and said to themselves, "Ah, I can escape my inputs!" without checking what my_escape actually did.

This code has been running in production for a few years. Nancy would like to change how it works, but it's not currently "an organizational priority" and constitutes a "high risk change to a stable system".

[TDWTF Survey Reminder] Got 5 minutes to tell your manager what you really think? Take the Developer Mentorship Survey and there just might be a free TDWTF mug in it for you.