martijnvanbrummelen / nwipe

nwipe secure disk eraser
GNU General Public License v2.0
786 stars 86 forks source link

Added ISAAC-64 PRNG for 64-bit systems. #398

Closed chkboom closed 2 years ago

chkboom commented 2 years ago

Use RANDIZL=8 as recommended by ISAAC author. Fixed possible buffer overflow in nwipe_isaac_read(). Slight performance improvement.

PartialVolume commented 2 years ago

Nice work, thanks for submitting this PR. I've yet to test it but will do so over the next few days. It's currently failing the formatting check. You need to run make format before doing another commit. I'm not sure how familiar you are with GitHub so my apologies if what I'm about to say you already know.

This is what I do when I forget to format the code after I've already committed the code and issued a pull request, for me, I find the quickest way to proceed is as follows:

  1. Delete the pull request
  2. On my PC run make format
  3. Recompile and do a quick check to make sure it runs.
  4. Now do a special commit that amends the changes to the last commit, thereby keeping the commit history, free of commits that just say 'opps, forgot to run make format'. The command to amend a commit to the last commit without showing in the history is git commit --amend --no-edit
  5. Push your branch to GitHub.
  6. Do a pull request.
chkboom commented 2 years ago

Weird, make format didn't give me any output. I've got another commit (relevant to this pull request) so I'll fix it up in that one. This is the output I get:

$ make format
clang-format -i -style=file src/*.c src/*.h
PartialVolume commented 2 years ago

That's correct make format doesn't output anything to the console, it just formats the files. If you want to see what it changed type make check_format. I think it's an underscore or maybe a hyphen. I'm not on my laptop so can't check the syntax at the moment

chkboom commented 2 years ago

"I'm not sure how familiar you are with GitHub" - Familiar enough that it drives me mad on a regular basis. I am guessing --no-edit doesn't open the editor? I never thought of this option, normally I just close the editor without changing anything, also does the trick.

I just pushed a new commit, maybe try this again. Formatting fixed, and ISAAC/ISAAC-64 PRNG performance improved by 300%.

PartialVolume commented 2 years ago

I just noticed that for some reason you don't have any workflows setup in your local branch. Normally my local branch would run workflows and I'd have them pass before doing a pull request. You don't need to do it the same but it does avoid creating PR's that fail as you already know they will pass from the workflow results of your local branch. You can then fix it before doing a PR. Maybe they don't exist in your branch because of github's requirement that for your first workflow it must be approved to run.

I can see from the code style in places where it's failing, if it ends up driving you mad trying to get the formatting fixed, let me know and I'll push the formatting changes to your branch, however, should be as simple as doing a 'make check-format` followed by a commit.

chkboom commented 2 years ago

Although assert is more succinct, I'd prefer all errors/warning/info to be send to nwipe_log as it makes it easier for users to just send the log file when reporting a bug or issue and we then know that all timestamped errors/warning will be in the log.

That makes sense, though I often use assert() is to document that it's a condition that is guaranteed never to occur, so will not pose a problem. If this is a problem let me know, changing to nwipe_log should be easy (though it adds more if statement mess).

I just noticed that for some reason you don't have any workflows setup in your local branch. Normally my local branch would run workflows and I'd have them pass before doing a pull request. You don't need to do it the same but it does avoid creating PR's that fail as you already know they will pass from the workflow results of your local branch. You can then fix it before doing a PR. Maybe they don't exist in your branch because of github's requirement that for your first workflow it must be approved to run.

I can see from the code style in places where it's failing, if it ends up driving you mad trying to get the formatting fixed, let me know and I'll push the formatting changes to your branch, however, should be as simple as doing a 'make check-format` followed by a commit.

I've never used the workflow thing before. I did a make check-format before committing this time, but it didn't change anything so I thought all was good. Should I amend and then force-push?

chkboom commented 2 years ago

I'm confused by the output of make check-format now. I manually edited it to how I thought the build tool wanted, when I run make check-format I get this:

$ make check-format
clang-format -i -style=file src/*.c src/*.h && git diff --exit-code
diff --git a/src/prng.c b/src/prng.c
index 017b0ee..2c4101d 100644
--- a/src/prng.c
+++ b/src/prng.c
@@ -36,7 +36,7 @@ static inline void u32_to_buffer( u8* restrict buffer, u32 val, const int len )
 {
     for( int i = 0; i < len; ++i )
     {
-        buffer[i] = ( u8 ) ( val & 0xFFUL );
+        buffer[i] = (u8) ( val & 0xFFUL );
         val >>= 8;
     }
 }
@@ -44,7 +44,7 @@ static inline void u64_to_buffer( u8* restrict buffer, u64 val, const int len )
 {
     for( int i = 0; i < len; ++i )
     {
-        buffer[i] = ( u8 ) ( val & 0xFFULL );
+        buffer[i] = (u8) ( val & 0xFFULL );
         val >>= 8;
     }
 }

It reverted to the style that's failing the test in the first place.

PartialVolume commented 2 years ago

This is what I get, seems to be the other one round:

make check-format
clang-format -i -style=file src/*.c src/*.h && git diff --exit-code
diff --git a/src/prng.c b/src/prng.c
index 65431f7..b4917cd 100644
--- a/src/prng.c
+++ b/src/prng.c
@@ -38,7 +38,7 @@ static inline void u32_to_buffer( u8* restrict buffer, u32 val, const int len )
 {
     for( int i = 0; i < len; ++i )
     {
-        buffer[i] = (u8) ( val & 0xFFUL );
+        buffer[i] = ( u8 )( val & 0xFFUL );
         val >>= 8;
     }
 }
@@ -46,7 +46,7 @@ static inline void u64_to_buffer( u8* restrict buffer, u64 val, const int len )
 {
     for( int i = 0; i < len; ++i )
     {
-        buffer[i] = (u8) ( val & 0xFFULL );
+        buffer[i] = ( u8 )( val & 0xFFULL );
         val >>= 8;
     }
 }
make: *** [Makefile:781: check-format] Error 1
chkboom commented 2 years ago

I changed it manually this time, because I'm not sure if the tool is doing the wrong thing or not. Edit: just saw the failed check again. Wow.

PartialVolume commented 2 years ago

Looks like it needs the space removed between ( ... )( .... )

clang-format -i -style=file src/*.c src/*.h && git diff --exit-code
make: *** [Makefile:781: check-format] Error 1
diff --git a/src/prng.c b/src/prng.c
index 7957c2c..72c9305 100644
--- a/src/prng.c
+++ b/src/prng.c
@@ -36,7 +36,7 @@ static inline void u32_to_buffer( u8* restrict buffer, u32 val, const int len )
 {
     for( int i = 0; i < len; ++i )
     {
-        buffer[i] = ( u8 ) ( val & 0xFFUL );
+        buffer[i] = ( u8 )( val & 0xFFUL );
         val >>= 8;
     }
 }
@@ -44,7 +44,7 @@ static inline void u64_to_buffer( u8* restrict buffer, u64 val, const int len )
 {
     for( int i = 0; i < len; ++i )
     {
-        buffer[i] = ( u8 ) ( val & 0xFFULL );
+        buffer[i] = ( u8 )( val & 0xFFULL );
         val >>= 8;
     }
 }
chkboom commented 2 years ago

Done, force pushed it. See if that helps.

PartialVolume commented 2 years ago

Done, force pushed it. See if that helps.

All good :-)

PartialVolume commented 2 years ago

@chkboom If there's no more commits to this PR, I'll rerun some of my own tests and merge.

chkboom commented 2 years ago

Go for it. Also i didn't get a chance to test on 32-bit, I can't build the ShredOS image, maybe I need to build it on a 32-bit system?

PartialVolume commented 2 years ago

Go for it. Also i didn't get a chance to test on 32-bit, I can't build the ShredOS image, maybe I need to build it on a 32-bit system?

Good point, yes, I'll probably compile it in a virtual machine on a 32 bit version of Debian but if you also want to also do that then go for it.

chkboom commented 2 years ago

Good point, yes, I'll probably compile it in a virtual machine on a 32 bit version of Debian but if you also want to also do that then go for it.

Unfortunately I don't have any 32-bit VMs set up at the moment. Might even be a good time to update the 32-bit images, to keep the repos in sync? I have a system with a Pentium M processor I can test it on.

PartialVolume commented 2 years ago

Good point, yes, I'll probably compile it in a virtual machine on a 32 bit version of Debian but if you also want to also do that then go for it.

Unfortunately I don't have any 32-bit VMs set up at the moment. Might even be a good time to update the 32-bit images, to keep the repos in sync? I have a system with a Pentium M processor I can test it on.

No problem, give me a day or so to test the 32 bit version and I'll either get back to you with the problems or merge the PR if no issues.

chkboom commented 2 years ago

The 32-bit flavour should default to ISAAC instead of ISAAC-64. I use sizeof( unsigned long int ) >= 8 but a compile-time method would be better. I couldn't find any reliable macro defining the native word length of the CPU.

In the past I found sizeof(int) is 4, even on a 64-bit system. Considering int should be the CPU native word length, that's a problem.

PartialVolume commented 2 years ago

The 32-bit flavour should default to ISAAC instead of ISAAC-64. I use sizeof( unsigned long int ) >= 8 but a compile-time method would be better. I couldn't find any reliable macro defining the native word length of the CPU.

In the past I found sizeof(int) is 4, even on a 64-bit system. Considering int should be the CPU native word length, that's a problem.

How about

#include <stdint.h>
#if UINTPTR_MAX == 0xffffffff
/* 32-bit */
#elif UINTPTR_MAX == 0xffffffffffffffff
/* 64-bit */
#else
/* wtf */
#endif

I've never used or tested this, courtesy of stack overflow.

PartialVolume commented 2 years ago

Although doesn't look like it works in all situations, see comments https://stackoverflow.com/questions/5272825/detecting-64bit-compile-in-c

PartialVolume commented 2 years ago

Working fine on 32bit Debian in Virtualbox. Defaults to Isaac (32bit) so no problems there. Ran about twenty wipes using all three PRNGs on a small 500MB virtual drive. Checked the drive with a hex editor. No issues. I won't bother giving any relative speeds as it's in a virtual machine which dramatically reduces I/O speed and the virtual drive is too small anyway. But so far all good. I'll merge this commit, then starting building with ShredOS which will give us more meaningful prng relative speed results.

PartialVolume commented 2 years ago

closes #396