Alleged Mt.Gox code leaked on IRC node by Russian Hacker
pastebin.com
pastebin.com
Some random red flags:
- There's a class with the name of the application. (Issues: Scope, SRP)
- There's a class with 1708 lines of code. (Scope)
- There's a switch-case statement that runs over 150 LOC (readability, maintainability)
- There's a string parsing function in the same class as transaction processing (Separation of concerns)
- There are segments of code commented out (are they not using source control?)
- There's inlined SQL (maintainability, security)
- There's JSON being generated manually & inline (SoC, DRY)
- There's XML being generated manually & inline (SoC, DRY)
- To sum up function _Route_getStats($path): XML production, JSON production, file writing, business logic, SQL commands, HTTP header fiddling, hard coded paging limits, multiple exit points...
The amount of refactoring needed here to bring this code up to acceptable quality is simply staggering.
What do PHP folk typically do for a big, hairy query that isn't really doable via an ORM? Not that this is the case here, just curious. I haven't touched PHP for years.
What we're actually talking about here which is really bad is string concatenation without parameterization.
Meaning, it seems the code is not using a database driver. PDO has been the accepted way of doing this. You pass the parameters to PDO separately from the query for sanitation. We can all see why this is bad:
if (isset($_GET['limit'])) {
$limit = (int)$_GET['limit'];
if ($limit < 1) $limit = 1;
if ($limit > 10000) $limit = 10000;
}
$req = 'SELECT * FROM `Money_Bitcoin_Node` WHERE `Status` = \'up\' AND `Last_Checked` > DATE_SUB(NOW(), INTERVAL 6 HOUR) AND `Version` >= 31500 AND (`Last_Down` IS NULL OR `Last_Down` < DATE_SUB(NOW(), INTERVAL 2 WEEK)) AND `First_Seen` < DATE_SUB(NOW(), INTERVAL 2 WEEK) ORDER BY RAND() LIMIT '.$limit;so... the statement above refactored to slightly better pseudo-code:
$limit = getLimitParameter();
$sqlString = SqlResourceLoader::Load("BitCoinNodeSelect.sql");
$statement = $db->prepare($sqlString);
$statement->bindValue(1, $limit, PDO::PARAM_INT);
$statement->execute();
..or the query with the SQL along with parameter parsing logic could be contained in a separate class altogether.Storing the query in a file means an additional IO call, right? Isn't the better approach to wrap the sql statement in a function if it's about to be reused?
I'm not arguing, I'd like to know the point and learn why saving that into a file helps maintenance.
Yes, there's a performance cost. Hard disks are fast, though. And then there's caching. I always value maintanability higher than performance, as maintainability often is correlated with number of bugs.
You suggest to wrap the SQL statement in a function, and that might be just as fine. There's no single best solution here. Just a lot of solutions that are better than the Bitcoin-class ;-)
What is wrong with stored procedures?
I'm sure there are good refutations to each, but these are commonly cited.
There is a classical old time running joke: "two typewriter girls talking - look, I could do 80 characters per minute, when I am in a mood. - bhaa, I could do 500.. but it is utter nonsense"
In some sense it summarizes all that "from zero to market in one month with cheap coders who just getting shit done" and other MVP-related nonsense.
Actually the code and how it was found speaks for it all.
Worse isn't always better.
When I start something from scratch, unlike a framework, I usually blast away in a single file, this just makes it easier for me in the prototyping phase. I will write a lot of "crappy" code, will not separate concerns but keep them close to the source for now and refactor it later. It helps me see some bigger pictures in my design, and allows for quick revision.
Luckily in these cases you will often find improvised version control systems in place (e.g. /site_old site_old123 /site_new2 /site :D)
> mail('mark@ookoo.org', 'BLOCK IMPORT ERROR', $e->getMessage()."\n\n".$e);
I haven't done much digging around this issue (so perhaps someone can enlighten me on this) but I was curious at what was at that domain (since it seemed the email was going to Mark Karpeles). When I went to the staff page[1] I could see his company Mutum Sigillum LLC takes care of the administration for that IRC network. That same staff page used his nickname MagicalTux and the corresponding page[2] links to his "Professionnal PHP5 certification"[3] but that lists his name as "Robert Karpeles" and not Mark Karpeles. Does anyone know why this is? Did he change his name between 2006 and now?
[1] http://en.wiki.gg.st/wiki/Staff [2] http://en.wiki.gg.st/wiki/MagicalTux [3] http://www.expertrating.com/transcript.asp?transcriptid=1005...
Edit: After more digging and looking at his test scores and code, I have come to the conclusion that this guy's skill level is not what he thinks it is. Here is my favourite: http://code.ohloh.net/file?fid=it28K0-Hdeyw2F2XDguAEU0ZSKI&c...
Notice his fix to stop local file inclusion vulnerabilities in PHP, apparently he has never heard of a null byte.
>mail('mark@tibanne.com,luke+eligius@dashjr.org', 'SSH connection to freetxn@'.$ip.' failed', 'Used ssh key 14a70b11-5f36-4890-82ca-5de820882c7f, but couldn\'t login to push those txs:'."\n".implode("\n", $el_todo));
(int)round($info['balance'] * 100000000)
such constructs are not a good sign. Quick glance over the rest of the code also confirms this. Way too much room for improvement (and that is a friendly way of saying it). >> 3 * 1005.01
=> 3015.0299999999997
This might get truncated to 3015.02 and might >> 3 * 1005.01 * 10000
=> 30150299.999999996
turn into a real loss.This is a good article about the subject: http://www.johndcook.com/blog/2009/04/06/numbers-are-a-leaky...
Also, that something is "well-defined" does not imply that is it not a mess. There are quite a few well-defined standards out there that clearly are a mess.
The IEEE-754 standard is not a mess. It is complicated, because it has to be. Extreme care and precision must be taken when talking about mathematical quantities which are neither closed nor associative under the provided operations.
I challenge anyone else to make a concrete standard which supports accurate transcendental operations with numbers which range from 0 to 10^300 while remaining simple and easy to understand.
I am not advocating their use to represent currency whatsoever, but the reasons are not because of the above, but rather the reason that I described.
They might be "well defined" but accurate they are not. In fact the whole point of them is that they are ONLY accurate to some degree.
Moreover, when you do floating point arithmetic, you are not adding the number "0.3" up three times. You are adding an approximation of "0.3" up three times, since 0.3 does not have an exact representation in binary.
What you're saying is akin to saying
"Well, I call 0.3333 + 0.3333 + 0.3333 = 0.9999 <> 1.0 inaccurate in my book."
This is totally accurate, and easier to see because it's in decimal.
The point of them is not that they are accurate to some degree. Floating point numbers have a direct correspondence to a real number. The "problem" is that the reverse is not true, so rounding has to occur.
You know that accurate is a synonym of exact, right?
1. accurate [noun] (of information, measurements, statistics, etc.) correct in all details; exact.
>Moreover, when you do floating point arithmetic, you are not adding the number "0.3" up three times. You are adding an approximation of "0.3" up three times, since 0.3 does not have an exact representation in binary.
As a CS graduate, I've known that for some decades. But that's at the floating point level, which is beside the point (no pun intented).
What the user wants to add is 0.3 -- he could even have entered that verbatim as his input. How it's done is irrelevant to him. What we're saying in this thread is that if that's what you want to do, and you're handling money, FP is not a good represenation.
>What you're saying is akin to saying "Well, I call 0.3333 + 0.3333 + 0.3333 = 0.9999 <> 1.0 inaccurate in my book."
No, what I'm saying is akin to saying:
"I call EXPLICITLY WRITING 0.3+0.3+0.3 and internally getting 0.3333 + 0.3333 + 0.3333 inaccurate in my book".
But sometimes you get the odd significance loss[1] situation.
Example, given floats a, b, and c
c = a - b
might result in
c == a
There are volumes of literature related to this.
The short explanation why it is so, is that there is an infinite range of numbers, but we do not have infinite amount of memory, Therefore, to perform operations on very large (or very small) numbers efficiently (on current computer architecture) you need to round things around.
If you do not have time for a degree in maths, then at least go and read this before operating on any instructions on floating point in production code, thanks:
"What Every Computer Scientist Should Know About Floating Point Arithmetic"
http://www.validlab.com/goldberg/paper.pdf
and
Now, with proper rounding in the right places, it will all work out. However, what rounding and where is much harder than using fixed point.
As long as it's just addition and subtraction, you'll mostly accumulate small errors -- however, once you multiply or compare numbers, all bets are off.
A good way to represent this is using Fowler's Quantity pattern:
http://martinfowler.com/eaaDev/quantity.html
That lets you encapsulate the ton of special behavior that money has. For example, for most software money can't be created or destroyed, just transfered.
I think we certainly have had PHP built infrastructure that scales, but surely you can see why an order matching system should not be written in PHP.
First of all, by doing it that way, the order matching system was coupled to the website, so now it makes perfect sense why the BitCoin price crashed after the DDoS attacks on MtGox. Because taking the website down meant taking the order matching down with it. No more trading.
From what I am seeing, this is not a case of PHP being evil (although, would you really run mission critical systems with PHP? The execution model doesn't make sense in that world and if you think a set_time_limit(0) on a PHP script is the same as an actual daemon written in a robust language meant for that execution model, then I think we are in extreme disagreement).
For me this is a case of a guy who's confidence was ahead of the reality. I'm sure in his mind a pacemaker running on PHP code is perfectly fine, and perhaps it might actually work for a while but that's just it, it will fail eventually (its a square peg in a round hole after all) and when it does it will be bad. We don't craft critical systems thinking of the best case scenario, we do it thinking of the worst and for my money, whatever happened at MtGox, its the worst case scenario.
PHP in Facebook, yes, NYSE? FUCK NO.
Also, for the GP, PHP's shared-nothing architecture means scaling horizontally is actually pretty easy all things considered. I just wouldn't write a bitcoin exchange in anything that doesn't have static typing: PHPs gotchas could quite literally cause a massive loss of money.
Languages do not scale, application designs do. You can even make BASIC scale if you design your app for that purpose.
Besides, nothing impressive about PHP running the "largest Bitcoin exchange". Bitcoin, in the grand scheme of things, is an insignificant niche, not even 1% of the world population has anything to do with it at the moment.
PHP runs much bigger stuff, like Yahoo! and of course Facebook (sure, Facebook now has custom PHP JITs et al, but used to run on just PHP back in the day when it was already huge).
I have used PHP on and off for years for many things. I'm not a PHP hater, but if it's not touching HTTP, it probably shouldn't be written in PHP.
[1] http://www.reddit.com/r/Bitcoin/comments/1x93tf/some_irc_cha...
Previous comments: https://news.ycombinator.com/item?id=7332372
/s
http://blog.magicaltux.net/2010/06/27/php-can-do-anything-wh...
Essentially he is a bad programmer and he has Not Invented Here (fixed, thanks jey) syndrome, both of which are terrible attributes for someone coding a Bitcoin exchange.
The code for the sshd does not seem to be there anymore, but from memory: it did not check if the number sent by Bob was 0, 1, or any any other groups that would make it easy solve the discrete logarithm problem. I don't think it bothered to check the primes either. [1] I think there was also something wrong with the signature checking (padding not checked maybe?).
Altogether it seemed like you could easily MITM connections made to the server, but I don't think I ever tried. It was a perfect example--to me at least--of why you should not spend a trivial amount of time reading about crypto on Wikipedia and then writing crypto code.
I can think of lots of reasons.
* You're exposed to any upstream bugs in the language and interpreter
* Dynamic typing and silent errors mean you're far more likely to make a mistake and not realize it
* It's missing a lot of battle tested libraries and frameworks
* It's PHP
[1]: http://me.veekun.com/blog/2012/04/09/php-a-fractal-of-bad-de...
How would you mitigate that, and how is the mitigation process any different for PHP?
public static function _Route_getStats($path) {
switch($path) {
case 'version':
...Did you mean statically typed? There are two dichotomies: static vs. dynamic, and weak vs. strong. Ruby and Python have strong dynamic typing. PHP has weak(-ish) dynamic typing.
For the purpose of short selling: who are they?
And (don't crucify me HN) I think dynamic typing rapidly loses its allure as your codebase grows large and "enterprise." The advantages in rapid prototyping and easy changes start to be overshadowed by the increasing burden added by unexpected side effects any time you change something. In Java, if I change an interface then I can find every client of that interface with one click and make sure each one is not broken by the change. The same is not true of JavaScript code; changing the valid inputs of a function can break things scattered around the code, and you won't notice unless you use a particular feature 3 times in a row on a waning gibbous moon.
Using them for what though? I doubt they use them for their transcation money handling code.
1. As in any language,
2. As in any interpreted language. Yes PHP has the gag operator (@) but it's use is discouraged (and not used in this example),
3. Not in my opinion. The Symfony framework alone has a ton of well used packages[0]. It does look like someone's decided to build their own framework from this example though,
4. Opinion.
Scheme, Standard ML, very early versions of Java, and BASIC come to mind.
Static typing really does seem particularly important in this domain, to the point that if an engineer is unable to find a suitable interpreted static language then the prudent choice would be to start looking at compiled langauges before relaxing the static typing requirement. Even implicit type conversion seems like a pretty questionable idea in this sphere. You really would want to be keenly aware of when you're doing something that might add or remove numerical precision.
Its syntax is bad and inconsistent, before 5.4 you couldnt have a function return an array and call it and access array contents like this funcName()['blah']. What the.
PHP only has one data type for structures, array, which in various ways pretends to be every data type, stack, queue, tree, hashmap, list, fuck it all, lets just call it array everywhere!
Anyway, everything else is implicitly converted as well, strings, integers, fuck it all, lets ouput 0 or 1 who cares.
PHP is basically not a language, its a clusterfuck of copy-pasted code everywhere, I bet its runtime, Zend, is as a copypaste of various C-libraries cobbled together.
I feel sorry for all the PHP lovers, must be stubborn ones, just like VisualBasic people 10 years ago when it was dying. PHP-lovers have the same argument now as VisualBasic did 10-15 years ago.
EDIT: Where and when I would use PHP. Never. There is always a better alternative. This talk about how easy it is to host, blah blah, then you never had to compile php and set its config flags and include dumb oci8 modules, pear or pecl files or whatever, or see it fail and no logs anywhere but you have to have system-level access to some ini files. Ugh... I think people have just learned to live with that crap, and the more sane languages and web frameworks like Flask or Grails are much easier.
- Collection classes like Vector (which is like an array in every other programming language ever) and Map (associative array or hashtable)
- Generics
- Lambda functions
- Asynchronous functions
- Type hinting for intrinsic types (int, string, bool)
Not much public documentation yet, but most bits are in the open source HHVM release, and there are a few blog posts on it (eg. http://www.sitepoint.com/hhvm-hack-part-1/)
As for not being able to access a array directly from a function which returns the array. It's just syntactical sugar. Yes long over due syntactical sugar however.
Autoupdateing can be implemented in VisualBasic as well.
As much as Lisps parenthesis everywhere do.
Want to get a web site up like right now, get some PHP. Want to run the largest Bitcoin exchange in the world, maybe not so much.
Having such a bad code in the worst language imaginable that eventually the site has been cracked by Russian punks.
This will be a great case study.
What a farce.