Hacker News Reader

Comment by layer8 | Hacker News Reader

The library doesn’t check for signed integer overflow here:

https://github.com/rxi/sj.h/blob/eb725e0858877e86932128836c1...

Certain inputs can therefore trigger UB.

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.

Hobby projects that prove useful have a tendency of starting to be used in production code, and then turning into CVEs down the road.

If there is a conscious intent of disregarding safety as you say, the Readme should have a prominent warning about that.

> Hobby projects that prove useful have a tendency of starting to be used in production code

Even if that is true, how is that the authors problem? The license clearly states that they're not responsible for damages. If you were developing such a serious project then you need the appropriate vetting process and/or support contracts for your dependencies.

I didn’t say it’s the author’s problem. It’s a problem with the code.

Why play all these semantic games? You're saying it's the author's problem. You want them to even edit their readme to include warnings for would be production/business users who don't want to pay for it.

GP is arguing about licences. Yes, formally there is no obligation, and I'm not saying the author has any such obligation.

In the present case, either the missing overflow check in the code is by mistake, and then it's warranted to point out the error, or, as I understood GGGP to be arguing, the author deliberately decided to neglect safety or correctness, and then in my opinion you can't reject the criticism as unwarranted if the project's presentation isn't explicit about that.

I'm not making anything the author's problem here. Rather, I'm defending my criticism of the code, and am giving arguments as to why it is generally good form to make it explicit if a project doesn't care about the code being safe and correct.

I understand your point and if I were the author I would want either a disclaimer or a fix. File an issue or make a pr. Filing an issue is quicker and more fruitful than dealing with folks here

It is useful to understand the limitations of such hobby programs to know what they are useful for.

> If there is a conscious intent of disregarding safety as you say, the Readme should have a prominent warning about that.

What do you consider this clause in the LICENSE:

>> THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

A standard clause you can find in every open source license? It doesn't say anything about how serious the project takes security

You write only Rust code don't you?

I wish ;) You're talking about how Rust code usually uses the MIT license and this is a part of the MIT license?

Every open source license has a very similar clause, include but not limited to BSD, GPL, CDDL, MPL and Apache.

then that is their problem, not the code author's. If you use a hobby project in production, that's on you

When such a library is used in production code, that's on the person who chose to use it in production, not on the original author of the library.

You are responsible for the code you ship, doesn't matter whether it's written by you, an LLM, or whether it's a third-party dependency.

While that is certainly true, we could also be nice and reduce the workload of someone reviewing their dependencies and write it down in the readme.

My personal take is: if the code is good enough, it should be trivial to switch to a better library at the point when needed.

> They acknowledge that these things aren't focused on "security" or "features" and that's okay.

where? single header is just a way to package software, it has no relation to features, security or anything such...

Either you are :

- overestimating the gravity of a UB and its security implications

- underestimate the value of a 150 line json parser

- or overestimate the feasibility of having both a short and high quality parser.

It sometimes happens that fixing a bug is quicker than defending the low quality. Not everything is a tradeoff.

I have tsoding fatigue. Took a long time to get him out of the main page. I like the DIY attitude, but it gets old really fast.

https://news.ycombinator.com/item?id=44345740

No one cares. Stop complaining or GTFO.

Au contraire, I think people do care. Now I will continue complaining and raising awareness with renewed fervor.

So if its a hobby project designed for just a handful of people, its suddenly okay to endanger them due to being sloppy?

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.

> The license also makes it clear that the authors aren't liable for any damages.

The license disclaims liability but that doesn't mean the author cannot ever be held liable. Ultimately, who is liable is up to a court to decide.

...and what open source software license in the world makes the author liable for damages?

None. That is how RedHat makes money.

Probably more of lack of explicit liability in the license.

Pretty sure the all caps text on the bottom of most open source licenses out there makes it clear

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

The code nor author don’t endanger anyone. Whoever uses it inappropriately endangers themselves or others.

Why are you using random, unvetted and unaudited code where safety is important?

Open Source is about sharing knowledge.

They are sharing their knowledge about how to create a tiny JSON parser. Where is the problem again?

Refer to the original comment. Seems like you are incapable of connecting the comment chain.

Have some manners please.

Yes, pretty much. It has enough of a warning.

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

[1]: https://news.ycombinator.com/item?id=45319399

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.

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.

Massively agree. Remember this thinking being everywhere with databases back in the day, not every text field is hooked up to a Wordpress comment section.

Untrusted doesn’t always mean adversarial IMO, even a bitrot can invalidate your entire input and possibly also trigger undefined behaviour if you aren’t prepared to handle that.

I was using a checksum to protect against "bitrot" since this was over a very noisy serial transmission line (over a slip ring). So, no, there was no "undefined behavior" and it's quite easy to avoid.

UB = "undefined behavior", thanks

I agree. I knew that the JSON is not going to change, so I wrote a 10 lines long parser for it. It is not a JSON parser by any means, but it parses properly what I need it to.

sscanf as a parser definitely has its uses

I used strchr() in a function I named get_pair(). So, all in all, I used strchr() and strcmp() only!

> Not everything needs to be absolutely 100% secure so long as you control the system.

Isn't that a bit like saying "you don't have to worry about home security as long as you are the only person who has the ability to enter your house"?

Sure. I don't password protect my (Android) TV like I password protect my (Android) phone, despite both of them allowing authorized access to the same Google accounts, because if someone entered my house I have bigger things to worry than them using my TV.

Not at all.

I mean yeah if you're truly the only person that has the ability to enter your house then why should you worry about home security? Nobody else has the ability to get in.

You probably didn't control the other end, as otherwise you would've used something more sane than JSON?

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).

For something that simple I'd choose a custom binary protocol or something like ASN.1 instead of JSON. It's easier to generate from a HLL and parse in a LLL (I've also been writing Asm for a few decades...)

I've done plenty of custom binary protocols before. I can't say they were any better or easier to deal with. I also can't say that the "parser" for a binary format was any easier than a simple, limited JSON parser.

For this specific project I chose JSON and it worked perfectly. Sending JSON from the embedded CPU was also really simple. Yes, there was a little overhead on a slow connection, but I wasn't getting anywhere near saturation. I think it was 9600 bps max on a noisy connection with checksums. If even 10% of the JSON "packets" got through it was still plenty for the system to run.

I would love to use ASN.1 if other programming languages would match up to Erlang's ASN.1. :(

Even if is not popular here .NET does support ASN.1, not sure if at the same level as Erlang.

It depends on the use case. JSON has a lot of tooling making it convenient in a lot of cases

Public facing interfaces are their own special thing, regardless if json or anything else, and not all data is a public facing interface.

If you need it, then you need it. But if you don't need it, then you don't need it. There is a non-trivial value in the smallness and simplicity, and a non-trivial cost in trying to handle infinity problems when you don't have infinity use-case.

This is a serialization library. The entire point is to communicate with data that's coming from out of process. It should be safe by default especially if it's adding a quick check to avoid overflow and undefined behavior.

Incorrect assumption.

If you are reading data from a file or stream that only you yourself wrote some other time, then it's true that data could possibly have been corrupted or something, but it's not true that it's automatically worth worrying about enough to justify making the code and thus it's bug surface larger.

How likely is the problem, how bad are the consequences if the problem happens, how many edge cases could possibly exist, how much code does it take to handle them all? None of these are questions you or anyone else can say about anyone else's project ahead of time.

If the full featured parser is too big, then the line drawing the scope of the lightweight parser has to go somewhere, and so of course there will be things on the other side of that line no matter where it is except all the way back at full-featured-parser.

"just this one little check" is not automatially reasonable, because that check isn't automatically more impoprtant than any other, and they are all "just one little checks"s. The one little check would perevent what? Maybe a problem that never happens or doesn't hurt when it does happen. A value might be misinerpreted? So what? Let it. Maybe it makes more sense to handle that in the application code the one place it might matter. If it will matter so much, then maybe the application needs the full fat library.

You would use this for parsing data you know is safe.

Using a "tiny library" for parsing untrusted data is where the mistake is. Not in OP code.

It's too bad this header-only JSON library doesn't meet your requirements. How much did you pay for your license to use it? I'm sure the author will be happy to either ship security fixes or give you a refund. You should reach out to him and request support.

There is only a problem with these checks here if you pass along arbitrarily large JSON strings as all of these counters are advanced at most once per input byte. If you don't limit the input to reasonable sizes you have a potentional denial of service problem even without the UB so you should be checking for reasonable sizes which depend on your application but are likely much lower than the 2^31-1 bytes the library can safely parse.

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.

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.

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.

If it has limitations they should be documented though right? especially if they’re security concerns.

If you review libraries, why do you need to quick scan the issues? You would have already identified all the issues right? Right?

Hahaha well said

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.

It's entirely reasonable for a tiny JSON parser written in C to not deal with JSON files over 0x7fffffff bytes long.

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)

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.

> 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#integ...

[2] https://rust-lang.github.io/rust-clippy/master/index.html#ar...

You can also just use ubsan if you want the runtime checks for overflows in C/C++, no need to switch languages for this.

Yes but those languages have defined overflow.

-fwrapv

Many other languages automatically switch to a big integer number type, or have arbitrary size integers anyway.

Which is a great way to turn an overflow into a denial of service as suddenly algorithms that were optimized for simple integer arithmetic slow to a crawl.

For signed overflow you can just turn on the sanitizer in trapping mode. Exhaustive checks is also not that terrible.

The amount of untrusted JSON I parse is very high.

UB is bad.

> 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.

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.

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.

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?

> 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.

    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

I can only hope this was made by an LLM and not a real human.

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

The depth is 32 bit, not the index into the file.

If you are nesting 2 Billion times in a row ( at minimum this means repeat { 2 billion times followed by a value before } another 2 billion times. You have messed up.

You have 4GB of "padding"...at minimum.

You file is going to be Petabytes in size for this to make any sense.

You are using a terrible format for whatever you are doing.

You are going to need a completely custom parser because nothing will fit in memory. I don't care how much RAM you have.

Simply accessing an element means traversing a nested object 2 billion times in probably any parser in the world is going to take somewhere between minutes and weeks per access.

All that is going to happen in this program is a crash.

I appreciate that people want to have some pointless if(depth > 0) check everywhere, but if your depth is anywhere north of million in any real world program, something messed up a long long time ago, never mind waiting until it hits 2 billion.

> I appreciate that people want to have some pointless if(depth > 0) check everywhere

An after the fact check would be the wrong way to deal with UB, you'd need to check for < INT_MAX before the increment in order to avoid it.

What is your definition of non-ancient? There are still embedded systems being produced today that don't have 32-bit integers.

And those will need careful review of any code you want to run on them because no one cares about your weird architecture nor should they have to.

I wouldn't call 8 or 16-bit microcontrollers (with no concept of a 32-bit int) that are in billions of devices "weird". But ok.

2 billion characters seems fairly plausible to hit in the real world

In a single line. Still not impossible, but people handling that amount of data will likely not have “header only and <150 lines” as a strong criteria for choosing their JSON parsing library.

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.

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).

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.

I've seen much bigger, though technically that wasn't valid json, but rather structured logging with JSON on each line. On the other hand, I've seen exported JSON files that could grow to such sizes without doing anything weird, just nothing exceeding a couple hundred megabytes because I didn't use the software for long enough.

Restricting the input to a reasonable size is an easy workaround for sure, but this limitation isn't indicated everywhere, so anyone deciding to consume this random project into their important code wouldn't know to defend against such situation.

In a web server scenario, 2GiB of { (which would trigger two overflows) in a compressed request would require a couple hundred kilobytes to two megabytes, depending on how old your server software is.

To be fair, anyone who uses a 150 line library without bothering to read it deserves what they get.

And in the spirit of your profile text I'm quite glad for such landmines being out there to trip up those that do blindly ingest all code they can find.

Not really. I deal with this everyday. If the library has a limit on the input size, it should mention this.

It is ~150 lines of code. Submit a PR, or when you git clone it add your checks, or stop complaining because the author does not owe you anything.

If you deal with this every day, you're an outlier.

For such big data, you should definitely be using an efficient format, not JSON.

I agree, but 2GB json files absolutely exist. It fits in ram easily

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.

Personally, all my C code is written with SEI C Coding Standard in mind.

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.

Can't use this library in production that's for sure.

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.

Submit a PR!

You don't get Hacker News karma if you quietly fix a bug instead of complaining about it, though.

I wouldn't expect a library like this to be secure. If you want it to be memory safe, compile it with Fil-C.

This has nothing to do with memory safety.

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.

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.

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.

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.

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).

cut a PR to improve it; that would be nice

Reply on news.ycombinator.com