Coding Horrors: You can gather all your technical debt into one easy framework!

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1501 »

Jeffrey of YOSPOS: Dec 22, 2005; GET LOSE, YOU CAN'T COMPARE WITH MY POWERS

1337JiveTurkey posted:

This'll probably horrify some posters, but I like:
code:
blah =	!isset(bravo)?	here(something) :
	isset(charlie)?	here(charlie) :
			nothere();
Conditions in one column, values in another and it's easy to quickly scan for what goes with what with the general else condition going at the end.

I like this, except maybe I'd line up the question marks and possibly the colons, especially if there were more cases.

# ? May 12, 2008 05:04

Adbot: ADBOT LOVES YOU

# ? Apr 26, 2024 21:14

theg sprank: Feb 3, 2008; pillage your village

1337JiveTurkey posted:

This'll probably horrify some posters, but I like:
code:
blah =	!isset(bravo)?	here(something) :
	isset(charlie)?	here(charlie) :
			nothere();
Conditions in one column, values in another and it's easy to quickly scan for what goes with what with the general else condition going at the end.

Were you at any point in your life a LISPer?

# ? May 12, 2008 20:55

Smackbilly: Jan 3, 2001; What kind of a name is Pizza Organ! anyway?

theg sprank posted:

I found these right next to each other in some code of mine today

code:

#define SIBLING(node) (((node)==((node)->parent->left)) ?\
                       ((node)->parent->right) : ((node)->parent->left))
#define GET_SIBLING(node) ((node) == (node)->parent->right ?\
			   (node)->parent->left : (node)->parent->right)

no idea what I was thinking

That's terrible not only for the redundancy but also for the fact that it sefaults your program if either node or parent is NULL.

# ? May 13, 2008 00:16

Scaevolus: Apr 16, 2007

Smackbilly posted:

That's terrible not only for the redundancy but also for the fact that it sefaults your program if either node or parent is NULL.

When you're writing tree structures, you can be pretty certain that that will never happen. Adding NULL checks to this would be stupid.

# ? May 13, 2008 00:36

1337JiveTurkey: Feb 17, 2005

Jeffrey posted:

I like this, except maybe I'd line up the question marks and possibly the colons, especially if there were more cases.

Extra formatting can make it more of a pain to change things later although the question mark would make a decent column delimiter.

theg sprank posted:

Were you at any point in your life a LISPer?

To be quite honest, I could never figure out what's proper indentation in Lisp for the life of me even if the other stuff makes sense.

# ? May 13, 2008 02:54

DaTroof: Nov 16, 2000; CC LIMERICK CONTEST GRAND CHAMPION
There once was a poster named Troof
Who was getting quite long in the toof

1337JiveTurkey posted:

This'll probably horrify some posters, but I like:
code:
blah =	!isset(bravo)?	here(something) :
	isset(charlie)?	here(charlie) :
			nothere();
Conditions in one column, values in another and it's easy to quickly scan for what goes with what with the general else condition going at the end.

Wow.

The first time I saw a ternary operator, it startled me. This startles me the same way. And much like a ternary operator, I bet it would make perfect sense the third or fourth time I came across it.

Neat idea. I don't know if I'll ever use it, but I like it.

# ? May 13, 2008 03:10

forelle: Oct 9, 2004; Two fine trout; Nap Ghost

Scaevolus posted:

When you're writing tree structures, you can be pretty certain that that will never happen. Adding NULL checks to this would be stupid.

Hahahah, you're joking right?

This is a VERY silly thing to say.

forelle fucked around with this message at 03:56 on May 13, 2008

# ? May 13, 2008 03:42

theg sprank: Feb 3, 2008; pillage your village

forelle posted:

Hahahah, you're joking right?

This is a VERY silly thing to say.

Come now, it's not that silly. These aren't general purpose routines, they're just shortcuts for when I would just write the entire thing out otherwise; basically, if the parent or the node itself is NULL where these are used, we have bigger problems somewhere else.

# ? May 13, 2008 04:22

JoeNotCharles: Mar 3, 2005; Yet beyond each tree there are only more trees.

A better question is why is this a define and not an inline function?

# ? May 13, 2008 04:25

theg sprank: Feb 3, 2008; pillage your village

JoeNotCharles posted:

A better question is why is this a define and not an inline function?

Not all standardized versions of C include inline functions.

Yes, I know, all the compilers that matter support it, etc., etc.

e: also to address some of the same concerns forelle had; the uppercase lettering says HEY DON'T USE THIS AS A FUNCTION TOO FREELY THIS IS TEXT REPLACEMENT. I dunno, I find that helpful sometimes.

theg sprank fucked around with this message at 04:33 on May 13, 2008

# ? May 13, 2008 04:29

forelle: Oct 9, 2004; Two fine trout; Nap Ghost

theg sprank posted:

Come now, it's not that silly. These aren't general purpose routines, they're just shortcuts for when I would just write the entire thing out otherwise; basically, if the parent or the node itself is NULL where these are used, we have bigger problems somewhere else.

I have a real objection to the "It can never be NULL here so I'm not going to check for it." idea. Adding checks tells me (the maintainer) that these pointers should never be NULL and if they are, something is hosed up.

You should always check every pointer before dereference through it, even if you believe with your whole heart it can never be NULL, because you're often wrong.

Also, is the parent of the root of the tree not NULL?

# ? May 13, 2008 04:40

theg sprank: Feb 3, 2008; pillage your village

forelle posted:

I have a real objection to the "It can never be NULL here so I'm not going to check for it." idea. Adding checks tells me (the maintainer) that these pointers should never be NULL and if they are, something is hosed up.

You should always check every pointer before dereference through it, even if you believe with your whole heart it can never be NULL, because you're often wrong.

Also, is the parent of the root of the tree not NULL?

The parent of the root of the tree is NULL. I do check for it. Here is the function in which the macro is used:

code:

static void
delete_cases(rbtree *theTree, rbnode *node)
{
  rbnode *sibling;
  /*Case 1:no rebalance if this is the only node in the tree!*/
  if(!node->parent) return;

  sibling = GET_SIBLING(node);
  /*more crap*/
}

For the record I agree with you, I just don't think the particular argument applies that much here. Meh, to each his own. I should find more bad code of mine to post in this thread and end the derail...

# ? May 13, 2008 04:56

Arms_Akimbo: Sep 29, 2006; It's so damn...literal.

I've been lurking this thread since the beginning, and I finally have something I can contribute.

I'm in the process of redesigning software at this growing small business to get it off Access w/ VBA and into SQL Server. The old VBA applets were written by some guy who is long gone. The code has all kinds of fun stuff in it, like entire blocks of code commented out and left in when he delivered it and seemingly random use of whitespace. But this one took the cake.

Every order is supposed to have a unique order number, obviously. One would think a simple auto-increment would solve the problem. But nope, let's do it in the longest way possible. There's a table that stores nothing but order numbers. Whenever a new order is created, the form calls this table, gets the last value, adds one to it, commits the new record, then uses that number for the new order.

When I get in the office tomorrow I'll try to post some actual code. It's beautiful.

# ? May 13, 2008 07:15

beuges: Jul 4, 2005; fluffy bunny butterfly broomstick

code:

#include "Named_Parameters.h"
#include "SMS_BUNDLE_TOPUPS_v1.h"
#include "SMS_Loyalty_Expiry.h"
#include "SMS_Loyalty_Expiry.c"

# ? May 13, 2008 10:16

MononcQc: May 29, 2007

Uberapan posted:

I noticed this one in my own code just now after getting strange reports of the time display not working correctly.

//Get the date and time
$date = date("Y-m-d H:m:s");

Hmm, I wonder why the times were off by a little

I was laughing at that pretty hard just yesterday, and about 30 minutes later I noticed I did the same thing in a script I wrote last week. :saddowns:

# ? May 13, 2008 12:58

Evil Angry Cat: Nov 20, 2004

MononcQc posted:

I was laughing at that pretty hard just yesterday, and about 30 minutes later I noticed I did the same thing in a script I wrote last week.

I've just noticed I did it in one of mine too haha.

# ? May 13, 2008 14:35

biznatchio: Mar 31, 2001; Buglord

forelle posted:

You should always check every pointer before dereference through it, even if you believe with your whole heart it can never be NULL, because you're often wrong.

There are plenty of situations where if an assumption like that is violated in the state of your data, the most sensible thing to do is segfault immediately.

# ? May 13, 2008 14:43

tef: May 30, 2004; -> some l-system crap ->

Here is a bit of a security coding horror:

http://lists.debian.org/debian-security-announce/2008/msg00152.html

quote:

Luciano Bello discovered that the random number generator in Debian's
openssl package is predictable. This is caused by an incorrect
Debian-specific change to the openssl package (CVE-2008-0166). As a
result, cryptographic key material may be guessable.

This is a Debian-specific vulnerability which does not affect other
operating systems which are not based on Debian. However, other systems
can be indirectly affected if weak keys are imported into them.

It is strongly recommended that all cryptographic key material which has
been generated by OpenSSL versions starting with 0.9.8c-1 on Debian
systems is recreated from scratch. Furthermore, all DSA keys ever used
on affected Debian systems for signing or authentication purposes should
be considered compromised; the Digital Signature Algorithm relies on a
secret random value used during signature generation.

The first vulnerable version, 0.9.8c-1, was uploaded to the unstable
distribution on 2006-09-17, and has since propagated to the testing and
current stable (etch) distributions. The old stable distribution
(sarge) is not affected.

Affected keys include SSH keys, OpenVPN keys, DNSSEC keys, and key
material for use in X.509 certificates and session keys used in SSL/TLS
connections. Keys generated with GnuPG or GNUTLS are not affected,
though.

Here is the fix: http://svn.debian.org/wsvn/pkg-openssl/openssl/trunk/crypto/rand/md_rand.c?op=diff&rev=300&sc=1

(Yes, it is just adding in a line that was commented out.)

And why did this happen? http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=363516

They wanted to get rid of warning messages caused by valgrind.

At this point I think this says it best:

Linus Torvalds posted:

Please, we do NOT fix compiler warnings without understanding the code! That's a sure way to just introduce _new_ bugs, rather than fix old ones. So please, please, please, realize that the compiler is _stupid_, and fixing warnings without understanding the code is bad.

# ? May 13, 2008 14:54

Smackbilly: Jan 3, 2001; What kind of a name is Pizza Organ! anyway?

tef posted:

Here is a bit of a security coding horror:

http://lists.debian.org/debian-security-announce/2008/msg00152.html

Here is the fix: http://svn.debian.org/wsvn/pkg-openssl/openssl/trunk/crypto/rand/md_rand.c?op=diff&rev=300&sc=1

(Yes, it is just adding in a line that was commented out.)

And why did this happen? http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=363516

They wanted to get rid of warning messages caused by valgrind.

At this point I think this says it best:

Maybe I'm just not understanding the code, but after giving it a brief glance it seems like the "correct" code is using the fact that uninitialized variables are not guaranteed to have any particular value as a source of entropy. Isn't that hugely non-portable? Is there anything the C spec that says that the compiler cannot zero out uninitialized variables? I know it doesn't have to, but is it forbidden to? If not, isn't the "correct" code insecure on any compiler which chooses to act this way?

# ? May 13, 2008 15:19

TSDK: Nov 24, 2003; I got a wooden uploading this one

tef posted:

They wanted to get rid of warning messages caused by valgrind.

Not sure if I'm reading the thread right, but are they really relying on uninitialised variables as a source of random data? That's a serious WTF right there, because there's absolutely nothing to say that uninitialised variables should contain non-predictable data.

tef posted:

At this point I think this says it best:

Linus Torvalds posted:
Please, we do NOT fix compiler warnings without understanding the code! That's a sure way to just introduce _new_ bugs, rather than fix old ones. So please, please, please, realize that the compiler is _stupid_, and fixing warnings without understanding the code is bad.

And that's a second WTF right there. Compilers are not stupid. Only people who think compilers are stupid, are stupid.

EDIT: Yes, I know I've just called Linus Torvalds stupid, but before the usual "Omg, but Linux, waaaaah!" starts up: I don't give a drat what he's done in the past, if he's going to say something stupid then I'll call it stupid.

TSDK fucked around with this message at 15:56 on May 13, 2008

# ? May 13, 2008 15:45

Smackbilly: Jan 3, 2001; What kind of a name is Pizza Organ! anyway?

TSDK posted:

And that's a second WTF right there. Compilers are not stupid. Only people who think compilers are stupid, are stupid.

Well, the admonition not to fix warnings without understanding the code is valid - in fact you shouldn't do anything to code that you don't understand. I think however that the better statement is not that the compiler is "stupid" but that it is conservative.

For example, consider

code:

if (x = y) {
 // stuff
}

Many compilers will emit a warning for this, because they can't possibly know if you made a typo, or really wanted to assign y to x and then test the value of x for truth. There's a way to get around this of course (extra parens) but that's an extra bit of otherwise meaningless code that exists only to placate the compiler. So in this sense the compiler is "stupid" because it can't tell what the code is logically supposed to be doing like a human programmer probably could. But really it's just being conservative and assuming that code that looks like a mistake is a mistake. Similarly, it could not tell that the programmer really did mean to use uninitialized data in the previous example, so it assumed it was a mistake.

# ? May 13, 2008 16:00

tef: May 30, 2004; -> some l-system crap ->

Smackbilly posted:

Maybe I'm just not understanding the code.

TSDK posted:

Not sure if I'm reading the thread right, but are they really relying on uninitialised variables as a source of random data?

The comment is wrong, and the warnings from purify *not valgrind - my bad* are spurious.

You can see that m is initialised by EVP_MD_CTX_init(&m); a few lines up, and buf is a parameter.

# ? May 13, 2008 16:45

TSDK: Nov 24, 2003; I got a wooden uploading this one

Smackbilly posted:

Well, the admonition not to fix warnings without understanding the code is valid - in fact you shouldn't do anything to code that you don't understand.

The admonishment may be correct for code that loads of other people are using and relying on is valid, but the reasoning is not.

I also disagree that you need to have a full and complete understanding of anything before coding or bug fixing. Often the only way to get to fully understand systems is when you actually go through it in practice trying to change or implement something. It's also perfectly possible to do an adequate job fixing bugs without understanding all of the surrounding code.

Smackbilly posted:

For example, consider
code:
if (x = y) {
 // stuff
}
Many compilers will emit a warning for this, because they can't possibly know if you made a typo, or really wanted to assign y to x and then test the value of x for truth. There's a way to get around this of course (extra parens) but that's an extra bit of otherwise meaningless code that exists only to placate the compiler.

It's not just the compiler you need to convince; if I saw that in code then I would go through the logic anyway to just to check that the coder hasn't made a genuine mistake. I imagine that most experienced coders would do likewise. The fully correct way to fix it is to put in the double parens, something you're unlikely to do by accident if it was a mistake, and ideally plonk a comment saying that you're doing it intentionally.

# ? May 13, 2008 16:48

Mikey-San: Nov 3, 2005; I'm Edith Head!

I think Torvalds means "stupid" in the sense that "it only knows a set of predefined rules". It cannot determine the intent of the programmer, so while it's good at what it does, it can't think. It's "dumb".

Yeah?

# ? May 13, 2008 17:06

TSDK: Nov 24, 2003; I got a wooden uploading this one

Mikey-San posted:

I think Torvalds means "stupid" in the sense that "it only knows a set of predefined rules". It cannot determine the intent of the programmer, so while it's good at what it does, it can't think. It's "dumb".

Yeah?

Whilst that is a reasonable statement about compilers, I'm still worried that someone's default attitude to warnings would be "I'm a programmer, I know best". Compiler warnings and sanity checking tools are there to help - as soon as false positives start creeping in unchecked, then they become useless. There's nothing worse than finding a bug the compiler was warning you about, but you didn't see it because it was hidden amongst a mass of 'harmless' warnings.

# ? May 13, 2008 17:17

Smackbilly: Jan 3, 2001; What kind of a name is Pizza Organ! anyway?

TSDK posted:

I also disagree that you need to have a full and complete understanding of anything before coding or bug fixing. Often the only way to get to fully understand systems is when you actually go through it in practice trying to change or implement something. It's also perfectly possible to do an adequate job fixing bugs without understanding all of the surrounding code.

I wasn't trying to say you do but you at least should understand the part of the code that you're fixing/changing, which it appears that this person didn't.

# ? May 13, 2008 17:20

Mikey-San: Nov 3, 2005; I'm Edith Head!

TSDK posted:

Whilst that is a reasonable statement about compilers, I'm still worried that someone's default attitude to warnings would be "I'm a programmer, I know best". Compiler warnings and sanity checking tools are there to help - as soon as false positives start creeping in unchecked, then they become useless. There's nothing worse than finding a bug the compiler was warning you about, but you didn't see it because it was hidden amongst a mass of 'harmless' warnings.

Yeah, it's a thorny bush. When can a programmer determine that a compiler warning is truly safe to ignore? At what point does the compiler know better, despite not "knowing" anything about the intent of the code? And how does the programmer know when which scenario is true?

# ? May 13, 2008 17:34

DaTroof: Nov 16, 2000; CC LIMERICK CONTEST GRAND CHAMPION
There once was a poster named Troof
Who was getting quite long in the toof

TSDK posted:

Whilst that is a reasonable statement about compilers, I'm still worried that someone's default attitude to warnings would be "I'm a programmer, I know best".

I think the default attitude that Linus implied is "Some other programmer wrote code that emits a warning, I better know why." He's talking about the possibility of introducing bugs through blind repair of compiler warnings, not advocating warning-prone code.

# ? May 13, 2008 17:37

trex eaterofcadrs: Jun 17, 2005; My lack of understanding is only exceeded by my lack of concern.

tef posted:

Here is a bit of a security coding horror:

This is a goddamn nightmare from the depths of hell itself. We have a lot of jobs that use scp, and a lot of Ubuntu servers. gently caress me

# ? May 13, 2008 18:26

Standish: May 21, 2001

So exactly how insecure are keys generated by Debian, is it actually a realistic vulnerability or is it a case of "should have taken billions of years to brute-force, now takes thousands of years"?

# ? May 13, 2008 18:42

Dessert Rose: May 17, 2004; awoken in control of a lucid deep dream...

I just turn on warnings as errors. Honestly, I consider code that emits warnings sloppy, no matter why. I suppose I haven't built apps that have cases where a warning is unavoidable, though.

# ? May 13, 2008 18:43

tef: May 30, 2004; -> some l-system crap ->

tef posted:

The comment is wrong, and the warnings from purify *not valgrind - my bad* are spurious.

You can see that m is initialised by EVP_MD_CTX_init(&m); a few lines up, and buf is a parameter.

It turns out I'm talking poo poo!

http://www.links.org/?p=327

Ben Laurie posted:

Usually it is bad to have any kind of dependency on uninitialised memory, but OpenSSL happens to include a rare case when its OK, or even a good idea: its randomness pool. Adding uninitialised memory to it can do no harm and might do some good, which is why we do it. It does cause irritating errors from some kinds of debugging tools, though, including valgrind and Purify. For that reason, we do have a flag (PURIFY) that removes the offending code. However, the Debian maintainers, instead of tracking down the source of the uninitialised memory instead chose to remove any possibility of adding memory to the pool at all. Clearly they had not understood the bug before fixing it.

So they fixed the code that added unitialised memory, by removing all calls that added things to the entropy pool.

tef fucked around with this message at 18:54 on May 13, 2008

# ? May 13, 2008 18:50

ShoulderDaemon: Oct 9, 2003; support goon fund; Taco Defender

Standish posted:

So exactly how insecure are keys generated by Debian, is it actually a realistic vulnerability or is it a case of "should have taken billions of years to brute-force, now takes thousands of years"?

It is a very real and serious vulnerability; keys built on Debian systems over the last two years have not had any realistic source of entropy.

# ? May 13, 2008 18:56

Moetic Justice: Feb 14, 2004; by Fistgrrl

Mikey-San posted:

I think Torvalds means "stupid" in the sense that "it only knows a set of predefined rules". It cannot determine the intent of the programmer, so while it's good at what it does, it can't think. It's "dumb".

Yeah?

But that's not "stupid," it's "not-smart."

# ? May 13, 2008 19:07

Standish: May 21, 2001

ShoulderDaemon posted:

It is a very real and serious vulnerability; keys built on Debian systems over the last two years have not had any realistic source of entropy.

Yeah but what exactly are the risks and attack vectors and how easy are they going to be to exploit, for example if I have a server which isn't affected by the bug itself but has a couple of pubkeys in authorized_keys2 which might have been generated on someone's Ubuntu desktop machine, is that server going to be part of a botnet by the time I get into work tomorrow morning?

Edit: yes, I know the keys need to be replaced regardless, I'm just curious if anyone has any concrete information as to how easily this can be exploited.

Standish fucked around with this message at 19:31 on May 13, 2008

# ? May 13, 2008 19:19

Mikey-San: Nov 3, 2005; I'm Edith Head!

Jungle Bus posted:

But that's not "stupid," it's "not-smart."

"Semantics"

# ? May 13, 2008 19:34

ShoulderDaemon: Oct 9, 2003; support goon fund; Taco Defender

Standish posted:

Yeah but what exactly are the risks and attack vectors and how easy are they going to be to exploit, for example if I have a server which isn't affected by the bug itself but has a couple of pubkeys in authorized_keys2 which might have been generated on someone's Ubuntu desktop machine, is that server going to be part of a botnet by the time I get into work tomorrow morning?

On Debian and Debian-derived systems, keys were being generated over a period of two years by a very large number of people, with a small-to-nonexistent entropy source. It is highly likely that there are currently people with deployed keys that are identical to your own. It's a little unclear how small the entropy pool actually way; I'm currently looking at some evidence that it may be as small as 32 to 48 bits. An initial guess was that there were a total of only 262144 keys generated over that entire period - 18 bits (!!!) - and the pool does appear to be heavily biased towards these keys on x86, at least.

It might not be this bad; but the way it looks right now is scary.

# ? May 13, 2008 20:02

Moetic Justice: Feb 14, 2004; by Fistgrrl

Mikey-San posted:

"Semantics"

In a programming forum? Surely you jest!

# ? May 13, 2008 20:12

Victor: Jun 18, 2004

You mean, a programming forum.

# ? May 13, 2008 20:20

Adbot: ADBOT LOVES YOU

# ? Apr 26, 2024 21:14

6174: Dec 4, 2004

tef posted:

It turns out I'm talking poo poo!

http://www.links.org/?p=327

So they fixed the code that added unitialised memory, by removing all calls that added things to the entropy pool.

What I find interesting from that links.org post is a lot of people are railing on Debian for not contributing the patch upstream, or at least asking upstream if it was an issue. Yet in the comments there is a link to a openssl-dev thread (http://marc.info/?t=114651088900003&r=1&w=2) where they did precisely that and no one mentioned it was a bad idea, and one of the responders, Ulf M�ller (listed as a dev team member on http://openssl.org/about/) said he was in favor of removing the calls. Seems to me that at least some of the blame for this should be on OpenSSL.

# ? May 13, 2008 20:27

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1501 »