Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

various issues cancel found in the c version, some of which pose security risks #6

Open
makoConstruct opened this issue Nov 16, 2018 · 6 comments

Comments

@makoConstruct
Copy link
Owner

  • restrict for strings
  • isStr has issue with casting to a non-compatible type under strict-aliasing
  • security issue with line buffer resizing, ensure that the line is not so large that the length value overflows. OOM error if it is.
  • replace most uint32_ts with usizes
  • reorder structs for packing
  • move the tag to outer struct?
  • remove freeIfSome, free already does that check
  • consider arena allocation for both rust and C versions
  • isStr should return a bool, otherwise it doesn't clarify anything
  • remove snowman
@makoConstruct makoConstruct changed the title various issues cancel found various issues cancel found in the c version, some of which pose security risks Nov 16, 2018
@finnoleary
Copy link

security issue with line buffer resizing, ensure that the line is not so large that the length value overflows. OOM error if it is.

Or just use rsize_t which is designed for that?

https://wiki.sei.cmu.edu/confluence/display/c/INT01-C.+Use+rsize_t+or+size_t+for+all+integer+values+representing+the+size+of+an+object

@makoConstruct
Copy link
Owner Author

Thanks for the note, fao. You're right. Hmm.. they seem to recommend rsize_t for a lot of cases, what's size_t for now? The only thing I can think of that they didn't give to rsize_t is, converting to and from pointers? Is that what it's for?

Looking back at the definition of Term_t, I'm not sure what I was thinking. I wanted them to be small but I also went with a ridiculous structpacking because I wanted the type tag to be at the front for some reason???

@finnoleary
Copy link

Thanks for the note, fao. You're right. Hmm.. they seem to recommend rsize_t for a lot of cases, what's size_t for now? The only thing I can think of that they didn't give to rsize_t is, converting to and from pointers? Is that what it's for?

So I dug around: size_t is for storing index values, as it's guaranteed to be large enough to store the maximum size of an array, or object. In this case object I think is anything you pass to sizeof -- indeed, that's the original purpose of size_t, to be guaranteed to hold whatever sizeof returns.

I was going to say something here, but at the last moment I got the impulse to double check, and quickly found this. So it looks like the correct type for that would be intptr_t, which I... instinctively remembered seeing, but couldn't find listed in the places I checked.

Looking back at the definition of Term_t, I'm not sure what I was thinking. I wanted them to be small but I also went with a ridiculous structpacking because I wanted the type tag to be at the front for some reason???

Mm, I think I've seen some LISP implementations that use tags, the type tag at the front I think is a shorthand for allowing you to access the first element of the struct by casting the pointer to that type and referencing it. It's a little weird, IMHO, but it's not completely ridiculous :)

@makoConstruct
Copy link
Owner Author

size_t is for storing index values

from the https://wiki.sei.cmu.edu/confluence/display/c/INT01-C.+Use+rsize_t+or+size_t+for+all+integer+values+representing+the+size+of+an+object

Any variable that is used to represent the size of an object, including integer values used as sizes, indices, loop counters, and lengths, should be declared rsize_t, if available. Otherwise, it should be declared size_t.

So, weird. Maybe something was just wrong with size_t and they don't recommend it for anything any more.

So it looks like the correct type for that would be intptr_t, which I... instinctively remembered seeing

Yeah.. I feel like I've seen that before as well. I wonder if rsize_t and intptr_t aren't coercible to each other, it sounds like they shouldn't be. That might be the benefit.

Hmm, no, looking into it, rsize_t is just a typedef, so no coercion protection. It also doesn't look like it's in the C++ standard? And the bounds checking associated with rsize_t only happens if you consciously use _s functions? https://en.cppreference.com/w/c/error

the type tag at the front I think is a shorthand for allowing you to access the first element of the struct by casting the pointer to that type and referencing it.

Hm, I'm convinced. I remember bringing this up with cancel ("Eskil Steenburg themself said that a pointer to a struct is always a pointer to its first element!") and apparently it's not in the C standards, but every relevant compiler does it and (clearly) so many users depend on it, I guess it's a de-facto standard behaviour at this point.

@finnoleary
Copy link

finnoleary commented Nov 25, 2018

So, weird. Maybe something was just wrong with size_t and they don't recommend it for anything any more.

Ah, yeah so. A little more digging suggests that indeed that purpose of size_t has been passed to rsize_t. This makes the primary purpose of size_t as being "able to represent the size of any object in bytes" (Stolen from the C++ reference, I didn't bother to check my copy of the C11 standard).

Yeah.. I feel like I've seen that before as well. I wonder if rsize_t and intptr_t aren't coercible to each other, it sounds like they shouldn't be. That might be the benefit.

I mean, semantically I can't think of an instance where you would want to coerce the one to the other unless you're doing base+offset indexing, but that inherently isn't something that necessarily causes bad code (Indeed, personally I find the alternatives, namely &(base[offset]), kind of ugly and more error prone in comparison), so I can't see why you'd really want to prevent that as a feature.

It also doesn't look like it's in the C++ standard?

C++ and C are different languages :)

Especially when you hit C11/C18 and C++14/C++17/C++20 etc. Standards-wise rsize_t is a part of C11 & C18 but not a part of C++ whatsoever.

Actually, while double checking I was correct on the above, I stumbled upon this SO thread you might find useful. It quotes the standard on the rationale of rsize_t versus size_t.

and apparently it's not in the C standards

The C11 standard seems to guarantee this. I dunno if it's guaranteed in earlier standards or specific to C11, though.

@makoConstruct
Copy link
Owner Author

Generally, the more specific we are about types, the more 'you put that into the wrong slot' errors we can spot, but, yeah, for that to do any good I think there would have to exist some function call that wants both a rsize_t and an intptr_t that a programmer could potentially get mixed up, and I can't think of any examples of that either (although that's not an API space I'd expect myself to know well)

Currently the code compiles under C and C++ mode, it would be convenient to keep it that way

The C11 standard seems to guarantee this

Ah yep. Turns out Cancel was talking about C++ standards back then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants