4 ms·

So this is a whole rant, and it's pretty much spot on probably, but the reader is left with a huge elephant of a question in the room: what is the proper way to

• asdfasgasdgasdg 8y ago
  > Why not show some C/C++ code samples of the proper way to do it, then? He provides constructive examples for all of the items that he criticizes. - Instead of pointer arithmetic, base_ptr, offset, len. - Instead of casting to internal structs, parsing into internal structs. - Instead of network byte order, buf[offset] + buf[offset+1] * 256 . - Instead of the wrong error codes, the right ones. - Instead of random asserts over internal data structures, validation at parse time.
  • clarry 8y ago
    > - Instead of network byte order, buf[offset] + buf[offset+1] * 256 . And you got it wrong. Shows that there is no silver bullet.
    • asdfasgasdgasdg 8y ago
      I mean, I don't actually know what the correct way to do this is, because it's been a long time since I thought about endianness. I just wrote down roughly what I remembered from the article. Also, of course there is no silver bullet. Everyone knows that RCEs are still achievable against Python or Java servers. But there are still better and worse ways to do things, and there are certain categories of error that you cannot make in Python and Java that you can make in C, where the opposite is rarely the case.
    • jjnoakes 8y ago
      It isn't wrong if that was the order the bytes were packed on the sending side.
    • alxlaz 8y ago
      That is quite obviously a misquote done in haste, because the article includes the correct form (buf[offset] * 256 + buf[offset + 1]) :-).
    • int_19h 8y ago
      The silver bullet is to use the standard library that provides appropriate abstractions, such as byte streams with functions like "read a 32-bit little-endian integer". Nobody should actually be writing code like the above in most cases; and in the very very few cases where they have to (e.g. when forced to program in an environment where these haven't been provided), they should write it exactly once as a reusable function, complete with appropriate tests.
      • tetha 8y ago
        And from experience, I find all of those tests for functions like this stupid, because the function is too trivial to write tests for. And then I always get the first 1 - 3 implementations wrong in some weird way, feel extremely stupid in the process and end up liking my tests.
  • paavoova 8y ago
    I wonder why the author chooses to write buf[offset]*256 + buf[offset+1] instead of the more common method to load bytes using bitwise arithmetic, e.g. b[k]<<8 | b[k+1]. This is far clearer instead of using potentially magic numbers. Does Javascript not have bitwise arithmetic? Why would you write C code with Javascript in mind?
    • michaelg7x 8y ago
      If b is uint8_t you get zero if you <<8, but the expected value if you * by (int)256; or is it late and I should keep quiet?
      • 0xffff2 8y ago
        The shift will automatically promote `b` to int, so the result is identical in both cases.
        • michaelg7x 8y ago
          Thanks. I'm mostly paranoid about whether the ROL instruction has precedence over the OR ... ;)
• camgunz 8y ago
  The main issue to me is that conversion, parsing, and validation are scattered throughout the code when they should be in separate, local, sequential steps. You should never be in a situation where you're halfway through allocating memory for an internal structure when suddenly you realize your input is malformed, for three reasons: - In order to make a good decision about what to do next, you need a lot of context: what you need to clean up, what any calling function expects you to do, any internal state you need to update, etc. You either end up passing a lot of extraneous context info around, or you make everything global. - Alternatively you bail with a status code and mutter "yolo" to yourself, or you throw an assert -- but crashing a system daemon with input is the essence of a DoS, so only the truly ignorant or lazy resort to it. - Building a reasonable framework is hard when all your concerns are entwined like this. If at some point you decide asserts on input are bad, it's very hard to change that convention in this code. Essentially all your function signatures and your entire call graph change. There's an extraordinary amount of technical debt here; just look at all the places Lennart had to change the size/offset stuff. The logic for this is scattered all over the place. People have focused on some of the tactical issues -- asserts on input, byte order conversion, etc. -- but the real issue is that when you write code in this style those things don't jump out at you because you have to do them everywhere. If your code had a better separation of concerns, as soon as you saw 'offsetof' in a parsing function you'd immediately see a red flag. It's so much easier to think in these specific contexts, where you're assured previous steps are complete, rather than in every single function having to get the entire conversion/validation/parsing flow correct every single time, keeping the entire thing in your head always.
• romed 8y ago
  I'm kinda bothered by the "C/C++" indistinction of your request. This code would look very, very different in C++.
  • stinos 8y ago
    Sorry about that, I just took over what the author used, even though I know there's actually no such thing as C/C++ and use of the term should die :[