dcleblanc / SafeInt

SafeInt is a class library for C++ that manages integer overflows.
MIT License
216 stars 37 forks source link

safeint and NT kernel/drivers vs. STL #22

Closed jaykrell closed 3 years ago

jaykrell commented 3 years ago

I'm trying to use current safeint.hpp essentially in NT kernelmode. There are a bunch of kinda trivial problems. We don't really have std:: of things, really trivial non-runtime things, and it is a fight to get it to compile. The integer types can be used w/o namespace. wchar_t is redundant with unsigned short in this environment, and errors; you likely understand.

Here is my initial hack/prototype, which would require some ifdef or something to fully upstream. I can iterate if there is tacit interest.

perl:

# Adapt current safeint.hpp to Windows kernelmode/libcntpr.
#
while (my $line = <>)
{
    $line =~ s/cstdint/stdint.h/g;
    $line =~ s/#include <cmath>/\/\/\0/g;
    $line =~ s/<limits>/<utl_limits.h>/g;
    $line =~ s/<type_traits>/<utl_type_traits.h>/g;
    $line =~ s/std::int/int/g;
    $line =~ s/std::uint/uint/g;
    $line =~ s/std::is_integral/utl::is_integral/g;
    $line =~ s/std::is_integral/utl::is_signed/g;
    $line =~ s/std::numeric_limits/utl::numeric_limits/g;
    $line =~ s/std::is_enum/utl::is_enum/g;
    $line =~ s/wchar_t/__wchar_t/g;
    print $line;
}
dcleblanc commented 3 years ago

What might work better for you would be to go back some number of versions before I switched to using things like std::uintXX_t. This of course has the problem that there have been a small number of actual fixes since then, so I’d prefer not to take that approach, except as a short-term fix. Speaking of old versions, last time I checked, there were a number of obsolete versions in use in Windows, if someone would go file bugs on them to get to latest, that would be a good thing.

As to the headers, that’s pretty easily resolved. All the includes and that sort of thing are done just after I sort out what compiler I have. We could easily make a #ifdef for Windows kernel builds that pulls in the headers you have, as opposed to the higher level C++ standard headers. It would be a good bit of work for me to build in that environment, as I’m no longer at Microsoft – I could go download the DDK and do something like that, but that isn’t exactly the NT build environment.

So the first thing is go look hard at the first 100 LOC where compiler capabilities are set, and come up with a good place to sort out how this fits with a kernel mode build, and things like constexpr – also exceptions – I did some recent work to get it to work better when C++ exceptions aren’t available, I suspect that will help you.

Then at line 174, there’s the headers, we can just #ifdef around those there. Note that this runs through line 212.

Also not sure how you want to handle exceptions, but since that’s very customizable, that should be one of the easier problems.

There’s a small amount of patching needed in safeint_internal::numeric_type, and actually, that’s where we could go grab the old code and make it optionally available.

I also had a test for this:

std::numeric_limits::is_signed

and the cleanest approach would be to make that an inline function that could be defined as either the old or the new way.

As to all the integers, I think the best approach there is that if you don’t have them, just define them. Like so:

ifdef SOMETHING

namespace std { typedef uint64 uint64_t; }

And so on – Before fixed size types were available in the standard, I just used the Microsoft-specific versions, which I presume you would have, and had a place to patch that up for other compilers.

Let me make you a branch we can work in for this.

From: Jay Krell notifications@github.com Sent: Saturday, December 5, 2020 6:59 AM To: dcleblanc/SafeInt SafeInt@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [dcleblanc/SafeInt] safeint and NT kernel/drivers vs. STL (#22)

I'm trying to use current safeint.hpp essentially in NT kernelmode. There are a bunch of kinda trivial problems. We don't really have std:: of things, really trivial non-runtime things, and it is a fight to get it to compile. The integer types can be used w/o namespace. wchar_t is redundant with unsigned short in this environment, and errors; you likely understand.

Here is my initial hack/prototype, which would require some ifdef or something to fully upstream. I can iterate if there is tacit interest.

perl:

Adapt current safeint.hpp to Windows kernelmode/libcntpr.

#

while (my $line = <>)

{

$line =~ s/cstdint/stdint.h/g;

$line =~ s/#include <cmath>/\/\/\0/g;

$line =~ s/<limits>/<utl_limits.h>/g;

$line =~ s/<type_traits>/<utl_type_traits.h>/g;

$line =~ s/std::int/int/g;

$line =~ s/std::uint/uint/g;

$line =~ s/std::is_integral/utl::is_integral/g;

$line =~ s/std::is_integral/utl::is_signed/g;

$line =~ s/std::numeric_limits/utl::numeric_limits/g;

$line =~ s/std::is_enum/utl::is_enum/g;

$line =~ s/wchar_t/__wchar_t/g;

print $line;

}

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/dcleblanc/SafeInt/issues/22, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHX6YL62HW3CBWPYJICMKILSTJDEZANCNFSM4UOTFQDQ.

jaykrell commented 3 years ago

Our environment sadly has no C++ exception support, and probably never will, so we will be happy to just have:

* inline bool SafeCast( const T From, U& To ) throw()
* inline bool SafeEquals( const T t, const U u ) throw()
* inline bool SafeNotEquals( const T t, const U u ) throw()
* inline bool SafeGreaterThan( const T t, const U u ) throw()
* inline bool SafeGreaterThanEquals( const T t, const U u ) throw()
* inline bool SafeLessThan( const T t, const U u ) throw()
* inline bool SafeLessThanEquals( const T t, const U u ) throw()
* inline bool SafeModulus( const T& t, const U& u, T& result ) throw()
* inline bool SafeMultiply( T t, U u, T& result ) throw()
* inline bool SafeDivide( T t, U u, T& result ) throw()
* inline bool SafeAdd( T t, U u, T& result ) throw()
* inline bool SafeSubtract( T t, U u, T& result ) throw()

I'm reluctant to stick to old versions. Cooperation is more than half the battle, so things are looking good. :)

I was unclear: We have one code base, where, depending on where in the code you are, it targets Win32 usermode, NT kernelmode, Linux usermode, and one special other thing. The NT kernelmode is C, so let's ignore it.

I'd prefer to have one safeint.hpp for all of it, possibly with different ifdefs/defines -- like, the Perl hack is acceptable, I'd still call using the original in two of the environments, and the hack in the third, as "one" safeint.hpp.

The "one special other thing" is usermode, but lives below ntdll, so uses libcntpr.lib. That is why no C++ exception handling and no STL.

A branch sounds good for my work style. :)

Let me iterate a bit on the Perl hack so I can shake it all out, and then work on ifdef'ing to be mergable.

I think though, inevitably, the result will be ugly.

In particular, I think it will be like this:

safeint_ntkernelmodeprefix.h: include utlfoo.h includde utlbar.h

safeint.hpp

ifdef safeint_ntkernelmode define safeintstd utl include safeint_ntkernelmodeprefix else include cmath include cstdint etc define safeintstd std endif

Or something like push_macro std define std utl .... pop_macro std

With the tacit understanding that user provides this utl thing. Kinda lame.

and ultimately it'll come down to what looks acceptably least disgusting, not the easier part of finding something that works.

Another alternative is you write all/most of the traits yourself. Which is a little gross, but mostly easy. One I don't know is _enum.

Also we don't need any float support.

Something I sent to others kinda applies here.

The UCRT/STL I think should be factored along these lines: exception vs. no exception header only vs. not ; maybe no matter, more about the next types and constants and constexpr vs. link/executed code (limits:max, is_integral, etc. being the first useful set) int vs. float

'cause, we'd be happy, big improvement, to just easily consume the int + header-only + no-exception parts. But to consume them, the other stuff gets in the way, of mere compilation.

If UCRT/STL was factored this way, then the int no-exception part of SafeInt.hpp would just work.

dcleblanc commented 3 years ago

Well, never is a long time, C++ exceptions used to have a lot of overhead, and now they are quite efficient, especially in x64. They used to say there would never be C++.

At any rate, there are other options – SafeInt has supported throwing structured exceptions since the beginning. There is also an option to call failfast. On non-Windows, if there’s no exceptions, you can just call abort. And since the exception handler is replaceable, you could do other things if you liked, just the exception handler cannot return.

The reason I bring that up is that the library compiles quite happily with a C++ compiler that has exceptions disabled, and you can use the class with some above solution, or the functions you list are there for exactly this purpose, but they too must at least compile if exceptions are disabled.

I agree with not sticking long term on old versions, just suggesting it if you need something immediately.

I really do not want to support perl hacks – here’s what I’d prefer:

if defined NO_CPP_HEADERS // or something

// Some other headers here

else

include

include

include // This is now required

// Need this for ptrdiff_t on some compilers

include

include // Needed for floating point implementation

endif

// Now go make work arounds because I don’t have type_traits, etc

This shouldn’t end up very ugly – this code existed long before we had any of the cool new standard stuff, and I had things implemented like so:

template bool is_signed(T) { return (T)-1 < 0; }

So I can revert to that, or conditionally revert to it for your case. Though having the std namespace defined does not imply you are using STL. So let’s see what we can do to manage things without the headers. Is your development environment publicly available (as I am no longer at Microsoft)? If so, it would go a bit easier, if not, we’ll manage.

From: Jay Krell notifications@github.com Sent: Saturday, December 5, 2020 4:34 PM To: dcleblanc/SafeInt SafeInt@noreply.github.com Cc: David LeBlanc dcl@dleblanc.net; Comment comment@noreply.github.com Subject: Re: [dcleblanc/SafeInt] safeint and NT kernel/drivers vs. STL (#22)

Our environment sadly has no C++ exception support, and probably never will, so we will be happy to just have:

I'm reluctant to stick to old versions. Cooperation is more than half the battle, so things are looking good. :)

I was unclear: We have one code base, where, depending on where in the code you are, it targets Win32 usermode, NT kernelmode, Linux usermode, and one special other thing. The NT kernelmode is C, so let's ignore it.

I'd prefer to have one safeint.hpp for all of it, possibly with different ifdefs/defines -- like, the Perl hack is acceptable, I'd still call using the original in two of the environments, and the hack in the third, as "one" safeint.hpp.

The "one special other thing" is usermode, but lives below ntdll, so uses libcntpr.lib. That is why no C++ exception handling and no STL.

A branch sounds good for my work style. :)

Let me iterate a bit on the Perl hack so I can shake it all out, and then work on ifdef'ing to be mergable.

I think though, inevitably, the result will be ugly.

In particular, I think it will be like this:

safeint_ntkernelmodeprefix.h: include utlfoo.h includde utlbar.h

safeint.hpp

ifdef safeint_ntkernelmode define safeintstd utl include safeint_ntkernelmodeprefix else include cmath include cstdint etc define safeintstd std endif

Or something like push_macro std define std utl .... pop_macro std

With the tacit understanding that user provides this utl thing. Kinda lame.

and ultimately it'll come down to what looks acceptably least disgusting, not the easier part of finding something that works.

Another alternative is you write all/most of the traits yourself. Which is a little gross, but mostly easy. One I don't know is _enum.

Also we don't need any float support.

Something I sent to others kinda applies here.

The UCRT/STL I think should be factored along these lines: exception vs. no exception header only vs. not ; maybe no matter, more about the next types and constants and constexpr vs. link/executed code (limits:max, is_integral, etc. being the first useful set) int vs. float

'cause, we'd be happy, big improvement, to just easily consume the int + header-only + no-exception parts. But to consume them, the other stuff gets in the way, of mere compilation.

If UCRT/STL was factored this way, then the int no-exception part of SafeInt.hpp would just work.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/dcleblanc/SafeInt/issues/22#issuecomment-739436220, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHX6YLZPMUMIQKFVAO77AELSTLGRBANCNFSM4UOTFQDQ.

dcleblanc commented 3 years ago

See the latest commit into the use_c_headers branch - I think this will work for you, if not, let me know.

jaykrell commented 3 years ago

I use the Perl not as a long term proposal per se, to you, but to kinda demonstrate the problems and something related to the fix. And likely I can prototype with it, and maybe run a while while we work out the upstream form, maybe.

The C++ exception handling thing is probably intractable. The overhead does not matter. There is just no available runtime and nobody is going to provide one. The usual C++ exception handling depends on kernel32/kernelbase and thread locals, all of which are not available. We do have C SEH but I don't think we'd use that for this. We don't use it in general. Failfast is inappropriate. bool or HRESULT/NTSTATUS is gross but the path of much less resistance, and fits the current code. C++ exceptions would be quite nice but they have been forever missing in NT kernel/drivers.

type_traits you can also handle just by specializing "every" type. I have already done that locally. Or your way, understood. The "utl" thing is just someone else maintaining the same. One small sticking point was is_enum, but I think an ok answer there is either: false, or ifdef mscver use intrinsic. This stuff is only for MSC really -- we'll use stock safeint.hpp on Linux.

Minor questions:

  1. Should the else be a separate file, so most people can omit it?
  2. Do we need to then indirect at compile time through another namespace, or should I just fill in std? I think both are ok, neither is great.
  3. The uint64_t etc, those can just be stdint.h and no std:: I think? The same form portably to everyone?
dcleblanc commented 3 years ago

I think our messages crossed – take a look at the latest check-in in the use_c_headers branch.

Assuming that the new C style headers I used work for you, you should be good to go. I do need to go add it to the build system, so that it builds both ways, and I also can build the test harness, but haven’t done a full build of all versions, or run it yet. And I have to go make builds on Linux to ensure I didn’t break anything with clang or gcc.

As to is_enum, I have no idea how that is implemented, or if it is even possible. So you’ll need to cast enums to ints if they happen to get passed to a SafeInt method, which is rare. The only part of this that was very much work was making a wrapper around numeric_limits, and changing the namespace. And remembering how I did it before in a way that didn’t annoy the compiler somehow, ended up going back to my original GitHub check-in for max.

From: Jay Krell notifications@github.com Sent: Monday, December 7, 2020 1:46 PM To: dcleblanc/SafeInt SafeInt@noreply.github.com Cc: David LeBlanc dcl@dleblanc.net; Comment comment@noreply.github.com Subject: Re: [dcleblanc/SafeInt] safeint and NT kernel/drivers vs. STL (#22)

I use the Perl not as a long term proposal per se, to you, but to kinda demonstrate the problems and something related to the fix. And likely I can prototype with it, and maybe run a while while we work out the upstream form, maybe.

The C++ exception handling thing is probably intractable. The overhead does not matter. There is just no available runtime and nobody is going to provide one. The usual C++ exception handling depends on kernel32/kernelbase and thread locals, all of which are not available. We do have C SEH but I don't think we'd use that for this. We don't use it in general. Failfast is inappropriate. bool or HRESULT/NTSTATUS is gross but the path of much less resistance, and fits the current code. C++ exceptions would be quite nice but they have been forever missing in NT kernel/drivers.

type_traits you can also handle just by specializing "every" type. I have already done that locally. Or your way, understood. The "utl" thing is just someone else maintaining the same. One small sticking point was is_enum, but I think an ok answer there is either: false, or ifdef mscver use intrinsic. This stuff is only for MSC really -- we'll use stock safeint.hpp on Linux.

Minor questions:

  1. Should the else be a separate file, so most people can omit it?
  2. Do we need to then indirect at compile time through another namespace, or should I just fill in std? I think both are ok, neither is great.
  3. The uint64_t etc, those can just be stdint.h and no std:: I think? The same form portably to everyone?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/dcleblanc/SafeInt/issues/22#issuecomment-740199047, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHX6YL3HNSBFO6SKZQQSLLDSTVELDANCNFSM4UOTFQDQ.

jaykrell commented 3 years ago

I would call this #if SAFEINT_HAS_TYPETRAITS or #if SAFEINT_HAS_STD_TYPETRAITS and #if SAFEINT_HAS_LIMITS or #if SAFEINT_HAS_STD_LIMITS

There is nothing really C about this imho. It is a wonky build environment that is hard to name, but, has these headers and they are so far troublesome to use.

dcleblanc commented 3 years ago

What we call it can be figured out later. I’m just trying to get something that works for right now. I’m trying to fight the temptation to name it SAFEINT_WONKY_MICROSOFT_ONLY_BUILD.

I’ll have time to address the wchar_t issue tomorrow, have to teach tonight.

From: Jay Krell notifications@github.com Sent: Monday, December 7, 2020 6:39 PM To: dcleblanc/SafeInt SafeInt@noreply.github.com Cc: David LeBlanc dcl@dleblanc.net; Comment comment@noreply.github.com Subject: Re: [dcleblanc/SafeInt] safeint and NT kernel/drivers vs. STL (#22)

I would call this

if SAFEINT_HAS_TYPETRAITS

or #if SAFEINT_HAS_STD_TYPETRAITS

There is nothing really C about this imho. It is a wonky build environment that is hard to name, but, has these headers and they are so far troublesome to use.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/dcleblanc/SafeInt/issues/22#issuecomment-740322567, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHX6YL55GRILXEZSEAROU7TSTWGUDANCNFSM4UOTFQDQ.

dcleblanc commented 3 years ago

Just fixed the wchar_t issue - this is another reason why I am conservative about this code base. This is reverting a change I made about a decade ago. I think this will fix your build issues completely, if not, let me know.

the 128-bit/intrinsics fix will go in seperately.

dcleblanc commented 3 years ago

Did some more checking to see if it will compile correctly on gcc, and clang - it did not, but that's now fixed. As it turns out, removing the copy constructor and assignment (or setting them to default) breaks things, because the default is not constexpr. So I reverted that change, and implemented both. I have just a bit more checking to do before I merge it into main branch, so if you could please test with latest, I'd appreciate it.

dcleblanc commented 3 years ago

@jaykrell - since you have not responded since December, I am closing this. Due to the fact that this change has gotten very large and very messy, and reverts standardization I put into place over a decade ago, I will not be merging this into the main branch. I will leave this branch as is.

jaykrell commented 3 years ago

Oh, darn, um, first I hadn't realized you were waiting for me, and then I forgot and have been away.

We discussed integer overflow a while on our project and decision was to abandon my similar but working new code and take yours once it got to master.

In the meantime, I have possibly made uint8_t et. al. work in our environment ('cause I was really after unique_ptr), but I am certain large swaths of code exist that will have that problem indefinitely. Our build system is a fork of a very heavily used system, and I had to make local hacks to get uint8_t to work but that I am sure others will not make. And the wchar_t problem is possibly more difficult to deal with and I don't plan and on trying to "fix" it. My point is, I think this work would actually benefit a lot of code, not just ours.