Register a SA Forums Account here!
JOINING THE SA FORUMS WILL REMOVE THIS BIG AD, THE ANNOYING UNDERLINED ADS, AND STUPID INTERSTITIAL ADS!!!

You can: log in, read the tech support FAQ, or request your lost password. This dumb message (and those ads) will appear on every screen until you register! Get rid of this crap by registering your own SA Forums Account and joining roughly 150,000 Goons, for the one-time price of $9.95! We charge money because it costs us money per month for bills, and since we don't believe in showing ads to our users, we try to make the money back through forum registrations.
 
  • Post
  • Reply
trex eaterofcadrs
Jun 17, 2005
My lack of understanding is only exceeded by my lack of concern.

6174 posted:

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.

I agree. This whole thing is a (drat annoying) lesson in how to behave in a public programming ecosystem, IMO.

Adbot
ADBOT LOVES YOU

Thom Yorke raps
Nov 2, 2004


This was really awesome.
code:
public class AwesomeData {
    List<Something> persons;
    int numberOfPersons = 0;
.
.
.
.
    public void setNumberOfPersons(int num) {
        numberOfPersons = num;
    }
    public int getNumberOfPersons() {
        return numberOfPersons;
    }
    public void addPersons(Something p) {
        persons.add(p)
    }
}
code:
public class UsesAwesomeData {
    AwesomeData ad;
.
.
.
    public void someRandomClass() {
        ad.addPersons(p);
        ad.setNumberOfPersons( ad.getNumberOfPersons() + 1 );
    }
}
In an over 10 million dollar project that has as a requirement that it must be very fast. To the point where we are running the program + the database on 48 servers with a tb of RAM. It is like they don't know what Collections are. This is from the same project from the OP.

hey wiz
Jun 18, 2005

It's gonna be even more awesome when two threads call someRandomClass() at the same time and end up setting numberOfPersons incorrectly due to a lack of synchronization.

That Turkey Story
Mar 30, 2003

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.

I completely disagree with this, unless you mean checks that only exist when compiling a debug build such as an assert. If by context your data can never be in a particular state, checking for that state in a release build every time that bit of code is run is just silly. For example, if I'm writing an iterator for something like a linked list, I'm not going to check at every increment or decrement if the pointer I'm dereferencing is NULL. Write proper unit tests rather than flooding your applications with redundant checks that you often can't do anything appropriate about if they fail anyway. If you really want to just inform the reader of the code that the data can't be NULL there as you claim, then use a comment rather than introducing a runtime check.

ShoulderDaemon
Oct 9, 2003
support goon fund
Taco Defender

That Turkey Story posted:

I completely disagree with this, unless you mean checks that only exist when compiling a debug build such as an assert. If by context your data can never be in a particular state, checking for that state in a release build every time that bit of code is run is just silly. For example, if I'm writing an iterator for something like a linked list, I'm not going to check at every increment or decrement if the pointer I'm dereferencing is NULL. Write proper unit tests rather than flooding your applications with redundant checks that you often can't do anything appropriate about if they fail anyway. If you really want to just inform the reader of the code that the data can't be NULL there as you claim, then use a comment rather than introducing a runtime check.

Why? If you're about to dereference a pointer in a dynamically-allocated structure, then there's a very good chance you've lost memory locality and are about to stall on memory access anyway, so the check is essentially free. It inflates code size slightly, so I wouldn't do it inside a tight loop, but I'd be trying not to traverse large structures in a tight loop anyway.

Steampunk Mario
Aug 12, 2004

DIAGNOSIS ACQUIRED

ShoulderDaemon posted:

Why? If you're about to dereference a pointer in a dynamically-allocated structure, then there's a very good chance you've lost memory locality and are about to stall on memory access anyway, so the check is essentially free. It inflates code size slightly, so I wouldn't do it inside a tight loop, but I'd be trying not to traverse large structures in a tight loop anyway.

Because as expensive as LHSes are, mispredicted branches can still hurt quite a bit on modern architecture. Remember, the fastest and least buggy code is the code that's not run, or ideally never written.

TSDK
Nov 24, 2003

I got a wooden uploading this one

tef posted:

It turns out I'm talking poo poo!
http://www.links.org/?p=327
It seems to me then that there were 3 WTFs:

1) Relying on specific, non-guaranteed behaviour from uninitialised memory.
2) Knowing that you get specific warnings about sections of code and failing to comment them appropriately.
3) Making a change to some code you don't fully understand without getting in touch with either the original author, or someone who could explain why it's doing what it is.

If you ask me, then the original OpenSSL devs are just as much to blame for that clusterfuck as the guy who put in an incorrect 'fix' for the problem.

Zombywuf
Mar 29, 2008

TSDK posted:

It seems to me then that there were 3 WTFs:

1) Relying on specific, non-guaranteed behaviour from uninitialised memory.
2) Knowing that you get specific warnings about sections of code and failing to comment them appropriately.
3) Making a change to some code you don't fully understand without getting in touch with either the original author, or someone who could explain why it's doing what it is.

If you ask me, then the original OpenSSL devs are just as much to blame for that clusterfuck as the guy who put in an incorrect 'fix' for the problem.

Interestingly the first fix posted (mentioned in the bug as not the right way to do it), would have been much less damaging. As for the idea of using uninitialised data as a source of entropy, it's risky as it's possible an attacker could control that data. The real WTF is that, from looking at the code, that buffer should have been filled from /dev/urandom and thus never triggered the warnings. It seems that it would only have used uninitialised data if /dev/urandom was unavailable.

forelle
Oct 9, 2004

Two fine trout
Nap Ghost

That Turkey Story posted:

I completely disagree with this, unless you mean checks that only exist when compiling a debug build such as an assert. If by context your data can never be in a particular state, checking for that state in a release build every time that bit of code is run is just silly. For example, if I'm writing an iterator for something like a linked list, I'm not going to check at every increment or decrement if the pointer I'm dereferencing is NULL. Write proper unit tests rather than flooding your applications with redundant checks that you often can't do anything appropriate about if they fail anyway. If you really want to just inform the reader of the code that the data can't be NULL there as you claim, then use a comment rather than introducing a runtime check.

Meh, I guess I'm just a very defensive programmer. I don't really trust anyone, most of all myself. I don't write or ship perfect code and if I can get meaningful messages back from the field ("NULL Pointer detected in blah, line blah") I'm a happy man. Even if it crashes, that message alone can be worth thousands of pounds in reduced debugging time. I've never seen an instance where checking for NULL caused a bug.

If I ever find that my application is running too slowly because I'm checking for NULLs I'll remove them where needed. Strangely I've never got to the point where that is my bottle neck.

But, this is derailing this thread and should probably be dropped.

tef
May 30, 2004

-> some l-system crap ->

TSDK posted:

3) Making a change to some code you don't fully understand without getting in touch with either the original author, or someone who could explain why it's doing what it is.

http://marc.info/?l=openssl-dev&m=114651085826293&w=2

An open ssl dev wrote in reply posted:

Kurt Roeckx schrieb:
> What I currently see as best option is to actually comment out
> those 2 lines of code. But I have no idea what effect this
> really has on the RNG. The only effect I see is that the pool
> might receive less entropy. But on the other hand, I'm not even
> sure how much entropy some unitialised data has.

Not much. If it helps with debugging, I'm in favor of removing them.
(However the last time I checked, valgrind reported thousands of bogus
error messages. Has that situation gotten better?)

HIERARCHY OF WEEDZ
Aug 1, 2005

I don't know if this counts as a coding horror, but it certainly makes me cry.

code:

SQL> DESC tblThings;
 Name                                      Null?    Type
 ----------------------------------------- -------- ----------------------------
 ...
 CREATED_DATE                                       DATE
 CREATED_TIME                                       VARCHAR2(50)
 REGISTERED_DATE                                    DATE
 REGISTERED_TIME                                    VARCHAR2(50)
 ACCESSED_DATE                                      DATE
 ACCESSED_TIME                                      VARCHAR2(50)

As near as I can figure, the default display of DATEs in Oracle (14-MAY-08) made one of the original devs think that it didn't store the time as well. So now - yeah. Imagine the number of fragile conversions that have to be done every time we need to parse that information.

And imagine my surprise when working with people who use Oracle who don't even understand the most basic tenets of it (I'll give you a hint: it was none)

Aredna
Mar 17, 2007
Nap Ghost

shopvac4christ posted:

I don't know if this counts as a coding horror, but it certainly makes me cry.

code:

SQL> DESC tblThings;
 Name                                      Null?    Type
 ----------------------------------------- -------- ----------------------------
 ...
 CREATED_DATE                                       DATE
 CREATED_TIME                                       VARCHAR2(50)
 REGISTERED_DATE                                    DATE
 REGISTERED_TIME                                    VARCHAR2(50)
 ACCESSED_DATE                                      DATE
 ACCESSED_TIME                                      VARCHAR2(50)

As near as I can figure, the default display of DATEs in Oracle (14-MAY-08) made one of the original devs think that it didn't store the time as well. So now - yeah. Imagine the number of fragile conversions that have to be done every time we need to parse that information.

And imagine my surprise when working with people who use Oracle who don't even understand the most basic tenets of it (I'll give you a hint: it was none)

Along those lines, most of the tables where I work have both date and time fields, but time is stored as seconds from midnight.

In some cases the devs couldn't be bothered to set up a time field so when we look at inventory transactions I have a date that they occured, but no idea to know the order that they occured in on that date. You would think you could use base it off the transaction id, but the ids are assigned when the open transaction is created and since several people post transactions there's no guarantee they were posted in the correct order (or for that matter it was even posted on the day it was created).

6174
Dec 4, 2004

TSDK posted:

If you ask me, then the original OpenSSL devs are just as much to blame for that clusterfuck as the guy who put in an incorrect 'fix' for the problem.

It seems that in the comments to that links.org post that the OpenSSL devs are trying to cast all the blame on the Debian guy. Comments are things like "Oh he shouldn't have contacted openssl-dev (like most open source projects and the OpenSSL support page/README with the source says to), he should have contacted this obscure nowhere published address". And "The Debian guy's proposed patch was presented without context so that is why we said it was ok, but patches like this are so common that we have an FAQ (which may or may not have existed 2 years ago) to explicitly say patches of this kind are a bad idea, so it is really the Debian guy's fault for not scouring the mailing list archive looking for similar patches".

The more I read coming from the OpenSSL team the more blame I think they have. It's too bad all the coverage I've seen of this (/. etc) don't mention OpenSSL's culpability at all.

Erasmus Darwin
Mar 6, 2001

TSDK posted:

It seems to me then that there were 3 WTFs:

1) Relying on specific, non-guaranteed behaviour from uninitialised memory.

Did they rely on it, though? I thought the behavior was, if present, a source of bonus entropy. If absent, the other sources of entropy were still added to the mix so they weren't any worse off. The problem (from what someone else mentioned) is that the Debian "fix" managed to disable all sources of entropy.

Anyway, I'm not sure if my understanding of this is correct as I've only really skimmed over the issue, but that's the impression that I got.

TheSleeper
Feb 20, 2003

Erasmus Darwin posted:

Did they rely on it, though? I thought the behavior was, if present, a source of bonus entropy. If absent, the other sources of entropy were still added to the mix so they weren't any worse off. The problem (from what someone else mentioned) is that the Debian "fix" managed to disable all sources of entropy.

Anyway, I'm not sure if my understanding of this is correct as I've only really skimmed over the issue, but that's the impression that I got.

Well they obviously are quite a bit worse off since taking the uninitialized data out as a source of entropy apparently led to weak keys.

zootm
Aug 8, 2006

We used to be better friends.

TheSleeper posted:

Well they obviously are quite a bit worse off since taking the uninitialized data out as a source of entropy apparently led to weak keys.
The guy removed both the section using uninitialised memory for entropy and a section which looked as though it was but which was not, which was the root of the problem. Check out the patch that caused the problem, and the fix.

Scaevolus
Apr 16, 2007

Regarding the OpenSSL vulnerability,

http://metasploit.com/users/hdm/tools/debian-openssl/

It looks pretty bad--

quote:

Removing this code has the side effect of crippling the seeding process for the OpenSSL PRNG. Instead of mixing in random data for the initial seed, the only "random" value that was used was the current process ID. On the Linux platform, the default maximum process ID is 32,768, resulting in a very small number of seed values being used for all PRNG operations.

All SSL and SSH keys generated on a Debian-based systems (Ubuntu, Kubuntu, etc) between September 2006 and May 13th, 2008 may be affected.

...

Q: How long did it take to generate these keys?
A: About two hours for the 1024-bit DSA and 2048-bit RSA keys for x86. I used 31 Xeon cores clocked at 2.33Ghz.

So for a given architecture and release, there's only 215 different keys. He completely brute-forced the two most commonly used keyspaces in about 62 CPU hours.

CT2049
Jul 24, 2007
I'll admit I've been doing the

if (... == true)

Now, before everyone jumps me I know this unnecessary. I've started doing it as I've worked with people who, when they review my code, find it much easier to follow if everything is explicitly defined. Before I started doing that I'd get asked a lot of questions explaining boolean conditions, since I've started explicitly stating them the questions have pretty much all stopped. I'm not sure why, but if it makes my life easier I guess I'll do it.

This is the same reason why I've taken to the habit of always prefacing a class-wide variable/functions with 'this'. I've had many time where reviewers will ask where a variable/function comes from and since I've started doing this those questions don't come up anymore.

The only thing I can think is that this improves readability for very very lazy reviewers.

Victor
Jun 18, 2004
It's best to have boolean variable names that sound boolean, like hidden (or even isHidden). I prefix nonpublic class fields with _ -- it's simple and to the point. With respect to calling a method inside a class, I'm a bit conflicted: if there's no this., the method invoked has to be one belonging to the enclosing type, at least in C#.

CT2049
Jul 24, 2007
I understand what you mean, but that's not what's happening here. I'd get questioned for writing
code:
if( foo.equals(foo2) )
but when I started writing like this:
code:
if( foo.equals(foo2) == true)
they'd all understand it. I have no idea, and I cringe everytime I do it, but it makes reviews of my code go faster.

The only thing I can think is that then you can glance and easly see if your checking for the true case or the false case, but even that is so simple the == shouldn't be necessary.

zootm
Aug 8, 2006

We used to be better friends.

CT2049 posted:

The only thing I can think is that then you can glance and easly see if your checking for the true case or the false case, but even that is so simple the == shouldn't be necessary.
Also it could be completely replaced by prefixing the condition with "!". There's really no excuse for the "== true" thing, it's dreadful style. :(

DICTATOR OF FUNK
Nov 6, 2007

aaaaaw yeeeeeah
Take a look at the source on this abomination of a website: http://forum.free-games.com.au/cgi-bin/gforum.cgi

As a web designer with borderline OCD, the code of that made me so sick that I ended up rewriting with perfect XHTML 1.0 Strict and CSS compliance just so I could look at it and feel at peace. The first thing I noticed was the lack of proper headers, followed by the horrendous whitespacing. After that, it sort of blurred together in one ball of poo poo.

I can only assume how terrible that forum software's codebase looks.

DICTATOR OF FUNK fucked around with this message at 23:22 on May 15, 2008

such a nice boy
Mar 22, 2002

CT2049 posted:

I understand what you mean, but that's not what's happening here. I'd get questioned for writing
code:
if( foo.equals(foo2) )
but when I started writing like this:
code:
if( foo.equals(foo2) == true)
they'd all understand it. I have no idea, and I cringe everytime I do it, but it makes reviews of my code go faster.

The only thing I can think is that then you can glance and easly see if your checking for the true case or the false case, but even that is so simple the == shouldn't be necessary.

That's ridiculous. The first one reads like an english sentence; who could possible have a hard time reading it? It's like the difference between saying "my name is Bob" and "my name is Bob is a true statement".

tef
May 30, 2004

-> some l-system crap ->
Today we made the switch from:

Manually incrementing version numbers in the file, and putting them in the commit log, and manually cleaning up the mess when the version numbers caused svn conflicts.

To:

Committing, and letting svn handle the version stamp in the file.

:toot:

I did a lap of victory around the office when it happened, I've been begging to do this since day one, over three months ago.

tef fucked around with this message at 00:31 on May 16, 2008

Victor
Jun 18, 2004
I think this looks best:
code:
if( true == foo.equals(foo2) == true)
(Just don't do the same with false.)

king_kilr
May 25, 2007

Victor posted:

I think this looks best:
code:
if( true == foo.equals(foo2) == true)
(Just don't do the same with false.)

I think there is something to be said for
code:
if ((foo == foo2) == true)

CT2049
Jul 24, 2007
Luckily after today I'm no longer working with the guys who needed me to do that. Hooray!

Although I'd like to try Victor's
code:
if( true == foo.equals(foo2) == true )
Just to see what they'd say... honestly I think they'd accept it.

bcrules82
Mar 27, 2003
rather than have an equals method why not just overload operator== ?
i see this sort of avoidance by old 'C' engineers all the time :)

Incoherence
May 22, 2004

POYO AND TEAR

bcrules82 posted:

rather than have an equals method why not just overload operator== ?
i see this sort of avoidance by old 'C' engineers all the time :)
I believe the answer you're looking for is "Java".

EssOEss
Oct 23, 2006
128-bit approved

bcrules82 posted:

rather than have an equals method why not just overload operator== ?
i see this sort of avoidance by old 'C' engineers all the time :)

== is generally defined as reference equality, not data equality. Overriding it to perform data comparison would be a Coding Horror.

Not to mention the need to give extra arguments to Equals() - case-sensitivity and comparison culture, in case of strings.

trex eaterofcadrs
Jun 17, 2005
My lack of understanding is only exceeded by my lack of concern.
There really should be an &= operator or something for referential equality.

MrMoo
Sep 14, 2000

TRex EaterofCars posted:

There really should be an &= operator or something for referential equality.

Well don't these operators map into the extra math symbols, we have =, ≡, and ≣, so you could have === (≡) as data equality, and ==== (≣) for class equality.

I don't have a degree in maths, when would you use "identical to" === (≡) and "strictly equivalent to" ==== (≣)? I just found this for possible usage in OOP:

http://weblog.raganwald.com/2008/04/is-strictly-equivalent-to.html

MrMoo fucked around with this message at 06:33 on May 16, 2008

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

MrMoo posted:

Well don't these operators map into the extra math symbols, we have =, ≡, and ≣, so you could have === (≡) as data equality, and ==== (≣) for class equality.

I don't have a degree in maths, when would you use "identical to" === (≡) and "strictly equivalent to" ==== (≣)? I just found this for possible usage in OOP:

http://weblog.raganwald.com/2008/04/is-strictly-equivalent-to.html

You're right, it would be &== and not &=.

As for the rest of that, I can't wait for Unicode keyboards. Using proper symbols would be a godsend, not some ==== bullshit. I know you can do Unicode in C# but typing in the codes is a pain in the dick.

TSDK
Nov 24, 2003

I got a wooden uploading this one

EssOEss posted:

== is generally defined as reference equality, not data equality. Overriding it to perform data comparison would be a Coding Horror.
No it wouldn't.

Begone, foul Java demon!

;)

zootm
Aug 8, 2006

We used to be better friends.

TSDK posted:

No it wouldn't.
In Java it would. Of course, it's not possible, so the point is somewhat moot. For what it's worth Scala uses "==" for (null-safe) value equality and "eq" for reference equality.

JoeNotCharles
Mar 3, 2005

Yet beyond each tree there are only more trees.
Clearly the correct naming scheme is equal?, eq?, and eqv?

king_kilr
May 25, 2007

EssOEss posted:

== is generally defined as reference equality, not data equality. Overriding it to perform data comparison would be a Coding Horror.

Not to mention the need to give extra arguments to Equals() - case-sensitivity and comparison culture, in case of strings.

Not necessarily, in python for example `is` is used for reference equality.

MagneticWombats
Aug 19, 2004
JUMP!

JoeNotCharles posted:

Clearly the correct naming scheme is equal?, eq?, and eqv?

code:
STk> (define a 3402354534)
a
STk> (define b 3402354534)
b
STk> (eq? a b)
#f
STk> (equal? a b)
#t
STk> (define c 1)
c
STk> (define d 1)
d
STk> (eq? c d)
#t

mlnhd
Jun 4, 2002

TRex EaterofCars posted:

As for the rest of that, I can't wait for Unicode keyboards. Using proper symbols would be a godsend, not some ==== bullshit. I know you can do Unicode in C# but typing in the codes is a pain in the dick.

What the hell is a Unicode keyboard? I am imagining a keyboard with over 100,000 buttons here.

Adbot
ADBOT LOVES YOU

Allie
Jan 17, 2004

Melonhead posted:

What the hell is a Unicode keyboard? I am imagining a keyboard with over 100,000 buttons here.

A keyboard with a compose key? Maybe four or five different compose keys?

Anything but ASCII in source code is infuriating as hell though. :)

  • 1
  • 2
  • 3
  • 4
  • 5
  • Post
  • Reply