7 ms·

The library doesn’t check for signed integer overflow here: https://github.com/rxi/sj.h/blob/eb725e0858877e86932128836c1f4e4f253cf8e4/sj.h#L89 https://github.c

• EmilStenstrom 1y ago
  Submit a PR!
  • uncircle 1y ago
    You don't get Hacker News karma if you quietly fix a bug instead of complaining about it, though.
• skydhash 1y ago
  There was a nice article [0] about bloated edge cases libraries (discussion [1]). Sometimes, it's just not the responsibility of the library. Trying to handle every possible errors is a quick way to complexity. [0]: https://43081j.com/2025/09/bloat-of-edge-case-libraries https://43081j.com/2025/09/bloat-of-edge-case-libraries [1]: https://news.ycombinator.com/item?id=45319399 https://news.ycombinator.com/item?id=45319399
  • layer8 1y ago
    The problem in the present case is that the caller is not made aware of the limitation, so can’t be expected to prevent passing unsupported input, and has no way to handle the overflow case after the fact.
    • skydhash 1y ago
      Do you not review libraries you add to your project? A quick scan of the issues page if it's on a forge? Or just reading through the code if it's small enough (or select functions)? Code is the ultimate specification. I don't trust the docs if the behavior is different from what it's saying (or more often fails to mention). And anything that deals with recursive structures (or looping without a clear counter and checks) is my one of the first candidate for checks. > has no way to handle the overflow case after the fact. Fork/Vendor the code and add your assertions.
      • layer8 1y ago
        Obviously I just did review it, and my conclusion was to not use that code. In the spirit of the article you linked, I’d rather write my own version.
      • jama211 1y ago
        If it has limitations they should be documented though right? especially if they’re security concerns.
      • knowitnone2 1y ago
        If you review libraries, why do you need to quick scan the issues? You would have already identified all the issues right? Right?
        • jama211 1y ago
          Hahaha well said
  • klysm 1y ago
    Strongly disagree here because JSON can come from untrusted sources and this has security implications. It's not the same kind of problem that the bloat article discusses where you just have bad contracts on interfaces.
    • leptons 1y ago
      JSON does not necessarily come from untrusted sources if you control the entire system. Not everything needs to be absolutely 100% secure so long as you control the system. If you are opening the system to the public, then sure, you should strive for security, but that isn't always necessary in projects that are not processing public input. Here's an example - I once coded a limited JSON parser in assembly language. I did not attempt to make it secure in any way. The purpose was to parse control messages sent over a serial port connection to an embedded CPU that controlled a small motor to rotate a camera and snap a photo. There was simply no way for any "untrusted" JSON to enter the system. It worked perfectly and nothing could ever be compromised by having a very simple JSON parser in the embedded device controlling the motor.
      • userbinator 1y ago
        You probably didn't control the other end, as otherwise you would've used something more sane than JSON?
        • leptons 1y ago
          I controlled both ends. There is nothing "insane" about JSON. It's used far and wide for many purposes. The system sending the JSON was based on Nodejs, so it was pretty natural to use JSON. And I did it with JSON just because I wanted to. I'd have had to invent some other protocol to do it anyway, and I didn't feel like reinventing the wheel when it was quite simple to write a basic JSON parser in assembly language, which is what I am comfortable with on the embedded system (been coding assembly for 40 years).
          • userbinator 1y ago
  • flykespice 1y ago
    There is no easy way out when you're working with C: either you handle all possible UB cases with exhaustive checks, or you move on to another language. (TIP: choose the latter)
    • jeroenhd 1y ago
      Very few programming languages default to checked increments. Most Rust or Java programmers would make the same mistake. Writing a function to do a checked addition like in other languages isn't exactly difficult, either.
      • paulddraper 1y ago
        Yes but those languages have defined overflow.
        • cozzyd 1y ago
          -fwrapv
      • deleted 1y ago
        [deleted]
      • koito17 1y ago
        > Most Rust or Java programmers would make the same mistake. Detecting these mistakes in Rust is not too difficult. In debug builds, integer overflow triggers a panic[1]. Additionally, clippy (the official linter of Rust), has a rule[2] to detect this mistake. [1] https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow https://doc.rust-lang.org/book/ch03-02-data-types.html#integ... [2] https://rust-lang.github.io/rust-clippy/master/index.html#arithmetic_side_effects https://rust-lang.github.io/rust-clippy/master/index.html#ar...
        • account42 1y ago
          You can also just use ubsan if you want the runtime checks for overflows in C/C++, no need to switch languages for this.
  • FooBarBizBazz 1y ago
    This might be the right attitude for a max function written in JavaScript, where the calling code has some control over the inputs. It's the wrong attitude for a JSON parser written in C, unless you like to get owned.
    • account42 1y ago
      It's entirely reasonable for a tiny JSON parser written in C to not deal with JSON files over 0x7fffffff bytes long.
  • paulddraper 1y ago
    The amount of untrusted JSON I parse is very high. UB is bad.
  • RossBencina 1y ago
    > Sometimes, it's just not the responsibility of the library. Sometimes. In this case, where the library is a parser that is written in C. I think it is reasonable to expect the library to handle all possible inputs. Even corner cases like this which are unlikely to be encountered in common practice. This is not "bloat" it is correctness. In C, this kind of bug is capable of being exploited. Sure, many users of this lib won't be using it in exposed cases, but sooner or later the lib will end up in some widely-used internet-facing codebase. As others have said, the fix could be as simple as bailing once the input size exceeds 1GB. Or it could be fine-grained. Either-way the fix would not "bloat" the codebase. And yes, I'm well aware of the single-file C library movement. I am a fan.
• ricardobeat 1y ago
  An int will be 32 bits on any non-ancient platform, so this means, for each of those lines: - a JSON file with nested values exceeding 2 billion depth - a file with more than 2 billion lines - a line with more than 2 billion characters
  • layer8 1y ago
    All very possible on modern platforms. Maybe more importantly, I won’t trust the rest of the code if the author doesn’t seem to have the finite range of integer types in mind.
    • johnisgood 1y ago
      Personally, all my C code is written with SEI C Coding Standard in mind.
  • klysm 1y ago
    2 billion characters seems fairly plausible to hit in the real world
    • naasking 1y ago
      2GB in a single JSON file is definitely an outlier. A simple caveat when using this header could suffice: ensure inputs are less than 2GB.
      • EasyMark 1y ago
        Or fork and make a few modifications to handle it? I have to admit I haven't looked at the code to see if this particular code would allow for that.
      • layer8 1y ago
        Less than INT_MAX, more accurately. But since the library contains a check when decreasing the counter, it might as well have a check when increasing the counter (and line/column numbers).
• odie5533 1y ago
  Can't use this library in production that's for sure.
  • deleted 1y ago
    [deleted]
• hypeatei 1y ago
  You're not aware of the simplistic, single header C library culture that some developers like to partake in. Tsoding (a streamer) is a prime example of someone who likes developing/using these types of libraries. They acknowledge that these things aren't focused on "security" or "features" and that's okay. Not everything is a super serious business project exposed to thousands of paying customers.
  • zwnow 1y ago
    So if its a hobby project designed for just a handful of people, its suddenly okay to endanger them due to being sloppy?
    • hypeatei 1y ago
      This is an open source project that you're not obligated to use nor did you pay for it. Who is it endangering? The license also makes it clear that the authors aren't liable for any damages.
      • flykespice 1y ago
        ...and what open source software license in the world makes the author liable for damages?
        • k_roy 1y ago
          Probably more of lack of explicit liability in the license.
          • virtue3 1y ago
            every OSS license I've ever seen is "use at your own risk" essentially. That's how this whole system works. You find a vulnerability? patch it, push change to repo maintainer. https://xkcd.com/2347 https://xkcd.com/2347
          • flykespice 1y ago
• oguz-ismail 1y ago
  diff --git a/sj.h b/sj.h index 60bea9e..25f6438 100644 --- a/sj.h +++ b/sj.h @@ -85,6 +85,7 @@ top: return res; case '{': case '[': + if (r->depth > 999) { r->error = "can't go deeper"; goto top; } res.type = (*r->cur == '{') ? SJ_OBJECT : SJ_ARRAY; res.depth = ++r->depth; r->cur++; There, fixed it
  • meindnoch 1y ago
    I can only hope this was made by an LLM and not a real human.
• habibur 1y ago
  Will trigger UB if level depth is > 2 billion or in the 2nd case number of lines > 2 billion. Limit you JS input to 1 GB. I will have more problems in other portions of the stack if I start to receive a 2 GB JSON file over the web. And if I still want to make it work for > 2GB, I would change all int in the source to 64 bits. Will still crash if input is > 2^64. What I won't ever do in my code is check for int overflow.
  • jcalvinowens 1y ago
    > What I won't ever do in my code is check for int overflow Amen. Just build with -fno-strict-overflow, my hot take is that should be the default on Linux anyway.
  • zelphirkalt 1y ago
    Crashing without a proper error message, leaving the user wondering what happened, is a table stake in C projects, of course. How do you intend to determine the cause of your crashes and write a meaningful error message for the user, in case of too long input when you don't check overflow?
• modeless 1y ago
  I wouldn't expect a library like this to be secure. If you want it to be memory safe, compile it with Fil-C.
  • layer8 1y ago
    This has nothing to do with memory safety.
    • modeless 1y ago
      This is an overstatement. Yes, UB does not necessarily cause a violation of memory safety, but triggering UB alone is not the goal of an attacker. UB is a means to an end and the end is usually a violation of memory safety leading to arbitrary code execution.
      • layer8 1y ago
        The primary point was that the code doesn't ensure correct processing (or returning an appropriate error) for all JSON. Even if behavior is defined by the C implementation, the overflow can lead to parser mismatch vulnerabilites, if nothing else. There are likely other "defined" failure modes the overflow can enable here. UB was a secondary observation, but it also can lead to logic errors in that vein, without involving memory safety. I'm not sure I agree that UB usually leads to memory safety violations, but in any case, the fact that signed integer overflow is UB isn't what makes the code incorrect and unsafe in the first place.
• robmccoll 1y ago
  Could just change the input len to an int instead of size_t. Not technically the correct type, but it would make it clear to the user that the input can't be greater than 2^31 in length.
• mscrnt 1y ago
  cut a PR to improve it; that would be nice
• Gibbon1 1y ago
  I've been tending to use ssize_t for indexes instead of int. Part of the reason was reading someones decent argument that for(int i=0; blah blah; i++) Is actually broken and dangerous on 64 bit machines.
  • oguz-ismail 1y ago
    How is ssize_t any better? It's not part of standard C and is only guaranteed to be capable of holding values between -1 and SSIZE_MAX (minimum 32767, no relation to SIZE_MAX).
• pgen 1y ago
  The author has kindly provided you with simple, readable, and free code. If you find it incomplete or unsafe, you can always modify it and contribute your changes if you wish to improve it, in accordance with the licence; and thank him while you're at it.
• Someone 1y ago
  I wouldn’t rate those as very serious issues for this project. They’ll only be triggered if there are over MAX_INT lines or depth levels in the input. Yes, an attacker might be able to do that, but you’d have to put that input in a memory buffer to call this code. On many smaller systems, that will OOM. Skimming the code, they also are loose in parsing incorrect json, it seems: static bool sj__is_number_cont(char c) { return (c >= '0' && c <= '9') || c == 'e' || c == 'E' || c == '.' || c == '-' || c == '+'; } case '-': case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': res.type = SJ_NUMBER; while (r->cur != r->end && sj__is_number_cont(*r->cur)) { r->cur++; } break; that seems to imply it treats “00.-E.e-8..7-E7E12” as a valid json number. case '}': case ']': res.type = SJ_END; if (--r->depth < 0) { r->error = (*r->cur == '}') ? "stray '}'" : "stray ']'"; goto top; } r->cur++; break; I think that means the code finds [1,2} a valid array and {"foo": 42] a valid struct (maybe, it even is happy with [1,2,"foo":42}) Those, to me, seem a more likely attack vector. The example code, for example, calls atoi on something parsed by the first piece of code. ⇒ I only would use this for parsing json config files. Being tiny is one thing, but the json grammar isn’t that complex. They could easily do a better job at this without adding zillions of lines of code.
• account42 1y ago
  Easy enough to fix: -sj_Reader sj_reader(char *data, size_t len) { +sj_Reader sj_reader(char *data, int len) { Not everyone needs to waste cycles on supporting JSON files larger than 2^31-1.