Skip to main content

A single line assignment filled with epic fail

Today I've been working with a piece of third-party software. I was having big problems getting it working and decided to delve into its source (which was included).

I'm not going to name the application (and I've obscured the details below), but the bug is so epic that it manages to combine on a single line four separate types of bugs: a design bug, two implementation bugs, a security hole and an out of date comment.

The program needs to copy a file into a directory. In the process of doing so it sets write permission on the directory into which the file will be copied. The first bug is the design: the program shouldn't be updating permissions on a directory without the user having requested the change. If the directory isn't writable an exception should be generated and an appropriate error displayed.

This particular JavaScript program avoided all that by just setting write permission. It did it like this:

dir.permissions = 666; // Make the file writable


dir
is an object that points to the directory to be written to and the permissions attribute allows direct 'chmod' style updating of UNIX-style permissions. Setting the permissions to octal 666 sets read/write for the owner of the directory, the group the owner belongs to and everybody else. It also removes execute permission. In the UNIX world execute permission on a directory means that you can CD into it; with it removed the directory cannot be accessed (i.e. you can't get a list of the files in the directory). That's bug number two.

The third bug is the security hole: by setting read and write for owner, group and everyone absolutely anyone can read and write to the directory that the user was trying to copy a file into. Imagine doing that on your My Documents or similar.

The fourth bug is that the implementer has accidentally used decimal 666 and not octal 666 (which should have been 0666) which means that they are doing the equivalent of octal 1232 which gives the owner of the directory just write permission, the group write and execute and everyone has permission to write. So the permissions are totally wrong.

The extra 1 at the beginning sets the sticky bit whose effect will be platform dependent.

And, finally, bug number five: the comment is incorrect. The permission change is happening on the directory and not the file. Worse, it's the permissions on the file that the coder was originally trying to change by introducing this one line change.

Comments

JMS said…
I argue the comment is correct. Comments should state the codes goal.
Jessica S. said…
I like the bugs that cause easily avoidable security problems. That way I can give coders a hard time for not knowing what they're doing, rather than the other way around...

Normally I'm at the receiving end of an ignorant programmer, being a user of software and all.
A devil of a line that.
SaltwaterC said…
Actually there is one bug: not using the octal notation.

The first "bug" is just a design choice. It's arguable if a situation where the error is recoverable makes sense to ask the user for permission. Sometimes it makes sense, sometimes it doesn't. It really depends on the application itself. One line of code is too little to tell.

The second "bug" is bogus. Not being able to list the directory contents is not the same as not being able to read / write files which was the actual intention. If it happens to know the file path, this is a non-issue. Really. Unless you want to find the path by walking the directory which is a totally different beast.

The third "bug" is contextual. Given certain applications, it may not be a bug at all. Even then, the defaults are 0755 and 0644 for new directories / files. This doesn't mean that somebody else may read the files of your home directory, unless your /home/foo is not set properly in the first place. If the dir happens to be inside another dir with 0700 permissions, then the hierarchy would deny the group and others to actually reach that level.

The fifth "bug", come on, since when the comments are "bugs"? Have you taken some time to actually read the definition of a software bug before posting this article? And the intention was actually to write something into that dir, namely files. It isn't that bad, actually.
Anonymous said…
@SaltwaterC: So, are you claiming John is unable to distinguish the context in the actual application?

By the way, you're wrong about bug 2. Testing:

$ mkdir john
$ echo test > john/file
$ cat john/file
test
$ chmod -x john
$ cat john/file
cat: john/file: Permission denied

In case you're wondering if cat lists the directory, let's see with strace: after some initialization calls, we get:

open("john/file", O_RDONLY|O_LARGEFILE) = -1 EACCES (Permission denied)

So no, even if you know the path, you can't read the file.
Marco Lopes said…
SaltwaterC wins the trophy for trolling!!!
Marco Lopes said…
SaltwaterC wins the trophy for trolling.
Here's why:

Bug 1-The first "bug" is just a design choice.

No it's not. Unless you're hacking a script to help you with some quick task, changing permissions on an arbitrary folder is wrong. The writer of the post already established we're talking about an application, so bug nr. 1.

Bug 2-Bug number 2 is a bug. An hardcoded permission that will prevent users from entering the folder or listing it's content is wrong. Either the developer didn't knew much about permissions (which seems to be the case given all the failures on that line of code) or, again, made a poor design choice.

Bug 3 - Actually the one you end up calling a bug, is not a bug per se, is just a security issue. It's still bad, and ends up being bad in a different way because of bug 4 causing the permissions to be set to something different that what first seem to be the case.

Bug 5 - In fact it isn't a bug, but it's quite bad, having comments describing a behaviour that is not what the code does, is misleading, it's noise and shouldn't be there.
Pete Hawkins said…
the main bug here is using JS server-side...
Ruben Berenguel said…
Amusing little line. Also a few amusing comments in the post... Congrats John ;) I hope you are not using this piece of software any more, beware of the lions waiting far below...

Ruben
Jordan said…
I agree with everything except the comment being labeled as a bug. While the comment did not reflect what was actually happening (ie changing permissions in directory), it did reflect what he wanted to accomplish, thus informing anyone reading it of his intended goals. It could have been more detailed but it describes what he was trying to do at least.
KK said…
What about umask? Does that not take care of bugs 1 and 2?
Chani said…
@SaltwaterC: please, tell me what programs you've contributed to, so I can NEVER USE THEM.
korina said…
In Unix, directories ARE files. His comment is technically accurate.
SaltwaterC said…
@id: yes, that was a confusion from my end. If the directory has the executable bit, then one can access / write the file by knowing the path, but listing is forbidden.

@Marco Lopez:

1. by your logic, every PO out there should remove the possibility to change permissions since that is a job for "scripts" and / or UNIX standard tools.

2. yes, it was a legit "bug".

3. the "security issues" are legit bugs. The worst kind. And even then, the hierarchy may avoid it.

5. wasn't the whole purpose of the comment for being ignored by the compiler / interpreter / etc? Since it can't affect the execution flow, it doesn't actually fit the description.

@Chani: please don't use them. In fact, nobody gives a shit. I may start to contribute to some that you use, just to piss you off.
SaltwaterC said…
@id: yes, that was a confusion from my end. If the directory has the executable bit, then one can access / write the file by knowing the path, but listing is forbidden.

@Marco Lopez:

1. by your logic, every PO out there should remove the possibility to change permissions since that is a job for "scripts" and / or UNIX standard tools.

2. yes, it was a legit "bug".

3. the "security issues" are legit bugs. The worst kind. And even then, the hierarchy may avoid it.

5. wasn't the whole purpose of the comment for being ignored by the compiler / interpreter / etc? Since it can't affect the execution flow, it doesn't actually fit the description.

@Chani: please don't use them. In fact, nobody gives a shit. I may start to contribute to some that you use, just to piss you off.

Popular posts from this blog

Your last name contains invalid characters

My last name is "Graham-Cumming". But here's a typical form response when I enter it:


Does the web site have any idea how rude it is to claim that my last name contains invalid characters? Clearly not. What they actually meant is: our web site will not accept that hyphen in your last name. But do they say that? No, of course not. They decide to shove in my face the claim that there's something wrong with my name.

There's nothing wrong with my name, just as there's nothing wrong with someone whose first name is Jean-Marie, or someone whose last name is O'Reilly.

What is wrong is that way this is being handled. If the system can't cope with non-letters and spaces it needs to say that. How about the following error message:

Our system is unable to process last names that contain non-letters, please replace them with spaces.

Don't blame me for having a last name that your system doesn't like, whose fault is that? Saying "Your last name …

All the symmetrical watch faces (and code to generate them)

If you ever look at pictures of clocks and watches in advertising they are set to roughly 10:10 which is meant to be the most attractive (smiling!) position for the hands. They are actually set to 10:09.14 if the hands are truly symmetrical. CC BY 2.0image by Shinji
I wanted to know what all the possible symmetrical watch faces are and so I wrote some code using Processing. Here's the output (there's one watch face missing, 00:00 or 12:00, because it's very boring):



The key to writing this is to figure out the relationship between the hour and minute hands when the watch face is symmetrical. In an hour the minute hand moves through 360° and the hour hand moves through 30° (12 hours are shown on the watch face and 360/12 = 30).
The core loop inside the program is this:   for (int h = 0; h <= 12; h++) {
    float m = (360-30*float(h))*2/13;
    int s = round(60*(m-floor(m)));
    int col = h%6;
    int row = floor(h/6);
    draw_clock((r+f)*(2*col+1), (r+f)*(row*2+1), r, h, floor(m…

The Elevator Button Problem

User interface design is hard. It's hard because people perceive apparently simple things very differently. For example, take a look at this interface to an elevator:


From flickr

Now imagine the following situation. You are on the third floor of this building and you wish to go to the tenth. The elevator is on the fifth floor and there's an indicator that tells you where it is. Which button do you press?

Most people probably say: "press up" since they want to go up. Not long ago I watched someone do the opposite and questioned them about their behavior. They said: "well the elevator is on the fifth floor and I am on the third, so I want it to come down to me".

Much can be learnt about the design of user interfaces by considering this, apparently, simple interface. If you think about the elevator button problem you'll find that something so simple has hidden depths. How do people learn about elevator calling? What's the right amount of informati…