The Daily WTF

CodeSOD: Taking Your Chances;

A few months ago, Sean had some tasks around building a front-end for some dashboards. Someone on the team picked a UI library for managing their widgets. It had lovely features like handling flexible grid layouts, collapsing/expanding components, making components full screen and even drag-and-drop widget rearrangement.

It was great, until one day it wasn't. As more and more people started using the front end, they kept getting more and more reports about broken dashboards, misrendering, duplicated widgets, and all sorts of other difficult to explain bugs. These were bugs which Sean and the other developers had a hard time replicating, and even on the rare occassions that they did replicate it, they couldn't do it twice in a row.

Stumped, Sean took a peek at the code. Each on-screen widget was assigned a unique ID. Except at those times when the screen broke- the IDs weren't unique. So clearly, something went wrong with the ID generation, which seemed unlikely. All the IDs were random-junk, like 7902699be4, so there shouldn't be a collision, right?

generateID() { return sha256(Math.floor(Math.random() * 100)). substring(1, 10); }

Good idea: feeding random values into a hash function to generate unique IDs. Bad idea: constraining your range of random values to integers between 0 and 99.

SHA256 will take inputs like 0, and output nice long, complex strings like "5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9". And a mildly different input will generate a wildly different output. This would be good for IDs, if you had a reasonable expectation that the values you're seeding are themselves unique. For this use case, Math.random() is probably good enough. Even after trimming to the first ten characters of the hash, I only hit two collisions for every million IDs in some quick tests. But Math.floor(Math.random() * 100) is going to give you a collision a lot more frequently. Even if you have a small number of widgets on the screen, a collision is probable. Just rare enough to be hard to replicate, but common enough to be a serious problem.

Sean did not share what they did with this library. Based on my experience, it was probably "nothing, we've already adopted it" and someone ginned up an ugly hack that patches the library at runtime to replace the method. Despite the library being open source, nobody in the organization wants to maintain a fork, and legal needs to get involved if you want to contribute back upstream, so just runtime patch the object to replace the method with one that works.

At least, that's a thing I've had to do in the past. No, I'm not bitter.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

The Watchdog Hydra;

Ammar A uses Node to consume an HTTP API. The API uses cookies to track session information, and ensures those cookies expire, so clients need to periodically re-authenticate. How frequently?

That's an excellent question, and one that neither Ammar nor the docs for this API can answer. The documentation provides all the clarity and consistency of a religious document, which is to say you need to take it on faith that these seemingly random expirations happen for a good reason.

Ammar, not feeling particularly faithful, wrote a little "watchdog" function. Once you log in, every thirty seconds it tries to fetch a URL, hopefully keeping the session alive, or, if it times out, starts a new session.

That's what it was supposed to do, but Ammar made a mistake.

function login(cb) { request.post({url: '/login', form:{username: config.username, password: config.password}, function(err, response) { if (err) return cb(err) if (response.statusCode != 200) return cb(response.statusCode); console.log("[+] Login succeeded"); setInterval(watchdog, 30000); return cb(); }) } function watchdog() { // Random request to keep session alive request.get({url: '/getObj', form:{id: 1}, (err, response)=>{ if (!err && response.statusCode == 200) return console.log("[+] Watchdog ping succeeded"); console.error("[-] Watchdog encountered error, session may be dead"); login((err)=>{ if (err) console.error("[-] Failed restarting session:",err); }) }) }

login sends the credentials, and if we get a 200 status back, we setInterval to schedule a call to the watchdog every 30,000 milliseconds.

In the watchdog, we fetch a URL, and if it fails, we call login. Which attempts to login, and if it succeeds, schedules the watchdog every 30,000 milliseconds.

Late one night, Ammar got a call from the API vendor's security team. "You are attacking our servers, and not only have you already cut off access to most of our customers, you've caused database corruption! There will be consequences for this!"

Ammar checked, and sure enough, his code was sending hundreds of thousands of requests per second. It didn't take him long to figure out why: requests from the watchdog were failing with a 500 error, so it called the login method. The login method had been succeeding, so another watchdog got scheduled. Thirty seconds later, that failed, as did all the previously scheduled watchdogs, which all called login again. Which, on success, scheduled a fresh round of watchdogs. Every thirty seconds, the number of scheduled calls doubled. Before long, Ammar's code was DoSing the API.

In the aftermath, it turned out that Ammar hadn't caused any database corruption, to the contrary, it was an error in the database which caused all the watchdog calls to fail. The vendor corrupted their own database, without Ammar's help. Coupled with Ammar's careless setInterval management, that error became even more punishing.

Which raises the question: why wasn't the vendor better prepared for this? Ammar's code was bad, sure, a lurking timebomb, just waiting to go off. But any customer could have produced something like that. A hostile entity could easily have done something like that. And somehow, this vendor had nothing in place to deal with what appeared to be a denial-of-service attack, short of calling the customer responsible?

I don't know what was going on with the vendor's operations team, but I suspect that the real WTF is somewhere in there.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

Error'd: Where to go, Next?;

"In this screenshot, 'Lyckades' means 'Succeeded' and the buttons say 'Try again' and 'Cancel'. There is no 'Next' button," wrote Martin W.


"I have been meaning to send a card, but I just wasn't planning on using PHP's eval() to send it," Andrew wrote.


Martyn R. writes, "I was trying to connect to PA VPN and it seems they think that downgrading my client software will help."


"What the heck, Doordash? I was expecting a little more variety from you guys...but, to be honest, I gotta wonder what 'null' flavored cheesecake is like," Joshua M. writes.


Nicolas wrote, "NaN exclusive posts? I'm already on the inside loop. After all, I love my grandma!"


[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!

CodeSOD: A Generic Comment;

To my mind, code comments are important to explain why the code what it does, not so much what it does. Ideally, the what is clear enough from the code that you don’t have to. Today, we have no code, but we have some comments.

Chris recently was reviewing some C# code from 2016, and found a little conversation in the comments, which may or may not explain whats or whys. Line numbers included for, ahem context.

4550: //A bit funky, but entirely understandable: Something that is a C# generic on the storage side gets
4551: //represented on the client side as an array. Why? A C# generic is rather specific, i.e., Java
4552: //doesn't have, for example, a Generic List class. So we're messing with arrays. No biggie.

Now, honestly, I’m more confused than I probably would have been just from the code. Presumably as we’re sending things to a client, we’re going to serialize it to an intermediate representation, so like, sure, arrays. The comment probably tells me why, but it’s hardly a necessary explanation here. And what does Java have to do with anything? And also, Java absolutely does support generics, so even if the Java trivia were relevant, it’s not accurate.

I’m not the only one who had some… questions. The comment continues:

4553: //
4554: //WTF does that have to do with anything? Who wrote this inane, stupid comment, 
4555: //but decided not to put comments on anything useful?

Not to sound judgmental, but if you’re having flamewars in your code comments, you may not be on a healthy, well-functioning team.

Then again, if this is someplace in the middle of your file, and you’re on line 4550, you probably have some other problems going on.

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

CodeSOD: A Random While;

A bit ago, Aurelia shared with us a backwards for loop. Code which wasn’t wrong, but was just… weird. Well, now we’ve got some code which is just plain wrong, in a number of ways.

The goal of the following Java code is to generate some number of random numbers between 1 and 9, and pass them off to a space-separated file.

StringBuffer buffer = new StringBuffer();
long count = 0;
long numResults = GetNumResults();

while (count < numResults)
	ArrayList<BigDecimal> numbers = new ArrayList<BigDecimal>();
	while (numbers.size() < 1)
		int randInt = random.nextInt(10);
		long randLong = randInt & 0xffffffffL;
		if (!numbers.contains(new BigDecimal(randLong)) && (randLong != 0))
			buffer.append(" ");
			numbers.add(new BigDecimal(randLong));
		System.out.println("Random Integer: " + randInt + ", Long Integer: " + randLong);	
	buffer = new StringBuffer();

Pretty quickly, we get a sense that something is up, with the while (count < numResults)- this begs to be a for loop. It’s not wrong to while this, but it’s suspicious.

Then, right away, we create an ArrayList<BigDecimal>. There is no reasonable purpose to using a BigDecimal to hold a value between 1 and 9. But the rails don’t really start to come off until we get into the inner loop.

while (numbers.size() < 1)
		int randInt = random.nextInt(10);
		long randLong = randInt & 0xffffffffL;
    if (!numbers.contains(new BigDecimal(randLong)) && (randLong != 0))

This loop condition guarantees that we’ll only ever have one element in the list, which means our numbers.contains check doesn’t mean much, does it?

But honestly, that doesn’t hold a candle to the promotion of randInt to randLong, complete with an & 0xffffffffL, which guarantees… well, nothing. It’s completely unnecessary here. We might do that sort of thing when we’re bitshifting and need to mask out for certain bytes, but here it does nothing.

Also note the (randLong != 0) check. Because they use random.nextInt(10), that generates a number in the range 0–9, but we want 1 through 9, so if we draw a zero, we need to re-roll. A simple, and common solution to this would be to do random.nextInt(9) + 1, but at least we now understand the purpose of the while (numbers.size() < 1) loop- we keep trying until we get a non-zero value.

And honestly, I should probably point out that they include a println to make sure that both the int and the long versions match, but how could they not?

Nothing here is necessary. None of this code has to be this way. You don’t need the StringBuffer. You don’t need nested while loops. You don’t need the ArrayList<BigDecimal>, you don’t need the conversion between integer types. You don’t need the debugging println.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.

CodeSOD: A Cutt Above;

We just discussed ViewState last week, and that may have inspired Russell F to share with us this little snippet.

private ConcurrentQueue<AppointmentCuttOff> lstAppointmentCuttOff { get { object o = ViewState["lstAppointmentCuttOff"]; if (o == null) return null; else return (ConcurrentQueue<AppointmentCuttOff>)o; } set { ViewState["lstAppointmentCuttOff"] = value; } }

This pattern is used for pretty much all of the ViewState data that this code interacts with, and if you look at the null check, you can see that it's unnecessary. Our code checks for a null, and if we have one… returns null. The entire get block could just be: return (ConcurrentQueue<AppointmentCuttOff>)ViewState["lstAppointmentCuttOff"]

The bigger glitch here is the data-type. While there are a queue of appointments, that queue is never accessed across threads, so there's no need for a threadsafe ConcurrentQueue.

But I really love the name of the variable we store in ViewState. We have Hungarian notation, which calls it a lst, which isn't technically correct, though it is iterable, so maybe that's what they meant, but if the point of Hungarian notation is to make the code more clear, this isn't helping.

But what I really love is that these are CuttOffs, which just sounds like some retail brand attempting to sell uncomfortably short denim. It'll be next year's summer trend, mark my words!

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

CodeSOD: Exceptional Standards Compliance;

When we're laying out code standards and policies, we are, in many ways, relying on "policing by consent". We are trying to establish standards for behavior among our developers, but we can only do this with their consent. This means our standards have to have clear value, have to be applied fairly and equally. The systems we build to enforce those standards are meant to reduce conflict and de-escalate disagreements, not create them.

But that doesn't mean there won't always be developers who resist following the agreed upon standards. Take, for example, Daniel's co-worker. Their CI process also runs a static analysis step against their C# code, which lets them enforce a variety of coding standards.

One of those standards is: "Catch specific exceptions. Don't catch the generic Exception type unless explicitly necessary." If it is explicitly necessary, their CI system attaches "Comments" (not code comments) to the commit, so all you need to do is click the "resolve" button and provide a brief explanation of why it was necessary.

This wouldn't be an… exceptional standard. Specific is always better than vague, and in this case, the rule isn't hard and fast: you're allowed to violate it if you can explain why.

But explaining yourself sounds like a lot of work. Wouldn't it be easier to try and fool the static analysis tool?

try { {...} } catch (Exception ex) when (ex is Exception exception) { {...} }

C#'s catch block supports a when operator, which is meant to filter based on properties of the exception. The ex is Exception exception is a pattern match and also a cast: it's true if the type of ex is Exception, and also cast it to Exception and store the cast in exception.

Or to word it another way, we catch the exception if it is of the type Exception but only when the type is Exception in which case we cast the exception which we know is an Exception to Exception and store it in exception, and I take exception to all of that.

Presumably, they'll change the rule in the CI system to exclude these, but hopefully they'll also have a talk with the developer responsible about the purpose of standards. Maybe they'll get some "standards by consent", or maybe somebody'll be looking for a new job.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.