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 bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. 6). Updates to Readme.md & Readme, notes for developers regarding configure options. 7). Wrong variable checked for NULL malloc pointer. 8). Fixes multiple memory leaks on malloc failure in method.c 9). Fixes multiple memory leaks on malloc failure in pass.c 10). Fix by cast: strlen expects const char * but gets unsigned char *. 11). Fixed another cast: strlen expects const char * etc. 12). Fixes uninitialized variable keystroke in nwipe_gui_verify function. 13). Fixes uninitialized variable keystroke in nwipe_gui_noblank function 14). Fixes implicit declaration of function 'fileno' in logging.c 15). Markup changes to README.md 16). Added required packages to README.md #46

Closed
wants to merge 45 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@PartialVolume
Copy link
Contributor

PartialVolume commented Sep 12, 2018

Correct "J=Up K=Down" text in method information footer in gui

Prior to this commit, when displaying the methods, the footer informa…
…tion bar displays the text J=Up K=Down Space=Select. This information is incorrected, J actually moves the selection down and K moves the selection up as defined in the code.

This commit corrects the footer bar message so it reflects the actual operation. Footer now reads J=Down K=Up

12/Sep/2018 20:55 N.Law
@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Sep 12, 2018

Prior to this commit, when displaying the methods, the footer information bar displays the text J=Up K=Down Space=Select. This information is incorrect, J actually moves the selection down and K moves the selection up as defined in the code.

This commit corrects the footer bar message so it reflects the actual operation. Footer now reads J=Down K=Up

@PartialVolume PartialVolume changed the title Prior to this commit, when displaying the methods, the footer informa… Correct "J=Up K=Down" text in method information footer in gui Sep 12, 2018

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Sep 12, 2018

Excuse the editing of the commit title, I'm a newbie when it comes to Github, learning as I go along. :-)

I've changed the description of this bug as the segmentation fault on…
…ly occurs if you exit nwipe without doing a wipe.

The problem is caused by the gui uninitialised thread ID, The gui thread is not created until after the wipe has started,
so if you control-C to exit nwipe without doing a wipe, "nwipe_gui_thread" would be uninitialised, which causes a
segmentation fault when "pthead_cancel" attempts to kill the gui thread using a value which isn't a real thread ID.
However by initialising "nwipe_gui_thread=0" and checking for 0 before calling "pthread_cancel" we avoid the
segmentation fault in this situation.

@PartialVolume PartialVolume changed the title Correct "J=Up K=Down" text in method information footer in gui Correct "J=Up K=Down" text in method information footer in gui AND Fix segfault on exit when not wiping. Sep 15, 2018

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Sep 15, 2018

Fixes #45, Fixes #47

Variable 'keystroke' is used uninitialized whenever function 'nwipe_g…
…ui_prng' is called.

The consequence of this uninitialised variable will be that intermittently pressing pressing 'P' won't do anything, that will occur if the uninitialised 'keystroke' happens to be equal to the defined value of ERR used in the while statement. This may or maynot be an extremely rare bug depending upon the unitialised contents of 'keystroke'. The fix is instead of using a while() loop, a do{}while() loop is used. Located in the function nwipe_gui_prng( void ) in gui.c

@PartialVolume PartialVolume changed the title Correct "J=Up K=Down" text in method information footer in gui AND Fix segfault on exit when not wiping. Various bug Fixes:Correct "J=Up K=Down" text in method information footer in gui AND Fix segfault on exit when not wiping. Sep 17, 2018

@PartialVolume PartialVolume changed the title Various bug Fixes:Correct "J=Up K=Down" text in method information footer in gui AND Fix segfault on exit when not wiping. Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng( void ) Sep 17, 2018

@PartialVolume
Copy link
Contributor Author

PartialVolume left a comment

Fixes #49

@PartialVolume
Copy link
Contributor Author

PartialVolume left a comment

Fixes #45

@PartialVolume
Copy link
Contributor Author

PartialVolume left a comment

Fixes #47

@PartialVolume
Copy link
Contributor Author

PartialVolume left a comment

This fixes a compiler generated warning regarding using an uninitialised value in the while loop.

Fixes non display of serial numbers for ATA/SCSI devices, does not fi…
…x serial numbers for USB devices, which will be fixed in a separate patch.

@PartialVolume PartialVolume changed the title Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng( void ) Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices Sep 24, 2018

@PartialVolume
Copy link
Contributor Author

PartialVolume left a comment

Fixes #50 for ATA/SCSI devives. Serial numbers now display in the GUI and the log.

The fix for USB serial numbers will be committed separately.

Fixes an intermittent segfault caused when you Control-C without star…
…ting any wipes. The structure variable 'nwipe_misc_thread_data->nwipe_selected' needed to be initialised to '0' as the wipe thread cancellation is based on this variable. If it contained an unitialised value as would be the case when no wipes had been started then the thread cancellation routine in the function signal_hand() would segfault.

@PartialVolume PartialVolume changed the title Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. Sep 25, 2018

Updates to README.md & README
regarding configure options for developers.

@PartialVolume PartialVolume changed the title Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. 6). Updates to Readme.md & Readme, notes for developers regarding configure options. Sep 25, 2018

PartialVolume added some commits Sep 25, 2018

Update README.md
Minor update
Wrong variable checked for NULL malloc pointer
In the function nwipe_ops2( ) in method.c, there are a number of mallocs. After each malloc the pointer returned by malloc is checked for NULL in order to determine whether there was a failure. In the case of one particular malloc the wrong variable is being checked.

@PartialVolume PartialVolume changed the title Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. 6). Updates to Readme.md & Readme, notes for developers regarding configure options. Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. 6). Updates to Readme.md & Readme, notes for developers regarding configure options. 7). Wrong variable checked for NULL malloc pointer Sep 26, 2018

@martijnvanbrummelen
Copy link
Owner

martijnvanbrummelen left a comment

Patch seems fine.

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Oct 2, 2018

That's great, thanks. I'm currently working on the --exclude patch, it's complete I'm just waiting for a series of tests on 32/64 bit systems and then I'll do a pull request.

After that, next on my list is a couple of patches for devices connected via USB e.g. 1. Serial No. not displayed. 2. There is something wrong in the code if you attempt to wipe a 2GB memory stick. As I fix each one I'll detail what the issue is.

Apart from that it's looking good, doing these fixes is helping me understand the code in more detail which will be of benefit when I write the certificate code.

There are a few issues as regards logging which I also want to fix, most importantly making it clear when a wipe has not completed successfully. This is important for the certificate creation as we don't want a certificate produced if the wipe ended prematurely.

Sorry about the number of commits, I hope you don't feel your getting spammed. I've got plenty of spare time to devote to this at the moment, which due to my job may not always be the case.

PartialVolume added some commits Oct 2, 2018

@PartialVolume PartialVolume changed the title Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. 6). Updates to Readme.md & Readme, notes for developers regarding configure options. 7). Wrong variable checked for NULL malloc pointer. 8). Fixes multiple memory leaks on malloc failure in method.c 9). Fixes multiple memory leaks on malloc failure in pass.c 10). Fix by cast: strlen expects const char * but gets unsigned char *. 11). Fixed another cast: strlen expects const char * etc. 12). Fixes uninitialized variable keystroke in nwipe_gui_verify function. 13). Fixes uninitialized variable keystroke in nwipe_gui_noblank function 14). Fixes implicit declaration of function 'fileno' in logging.c Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. 6). Updates to Readme.md & Readme, notes for developers regarding configure options. 7). Wrong variable checked for NULL malloc pointer. 8). Fixes multiple memory leaks on malloc failure in method.c 9). Fixes multiple memory leaks on malloc failure in pass.c 10). Fix by cast: strlen expects const char * but gets unsigned char *. 11). Fixed another cast: strlen expects const char * etc. 12). Fixes uninitialized variable keystroke in nwipe_gui_verify function. 13). Fixes uninitialized variable keystroke in nwipe_gui_noblank function 14). Fixes implicit declaration of function 'fileno' in logging.c 15). Markup changes to README.md Oct 2, 2018

@PartialVolume PartialVolume changed the title Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. 6). Updates to Readme.md & Readme, notes for developers regarding configure options. 7). Wrong variable checked for NULL malloc pointer. 8). Fixes multiple memory leaks on malloc failure in method.c 9). Fixes multiple memory leaks on malloc failure in pass.c 10). Fix by cast: strlen expects const char * but gets unsigned char *. 11). Fixed another cast: strlen expects const char * etc. 12). Fixes uninitialized variable keystroke in nwipe_gui_verify function. 13). Fixes uninitialized variable keystroke in nwipe_gui_noblank function 14). Fixes implicit declaration of function 'fileno' in logging.c 15). Markup changes to README.md Various bug Fixes: 1). Correct "J=Up K=Down" text in method information footer in gui. 2).Fix segfault on exit when not wiping. 3). Fix unitialised 'keystroke' variable in function nwipe_gui_prng(). 4). Fix serial numbers for ATA/SCSI devices. 5). Fixes an intermittent segfault when you CONTROL-C without starting any wipes. 6). Updates to Readme.md & Readme, notes for developers regarding configure options. 7). Wrong variable checked for NULL malloc pointer. 8). Fixes multiple memory leaks on malloc failure in method.c 9). Fixes multiple memory leaks on malloc failure in pass.c 10). Fix by cast: strlen expects const char * but gets unsigned char *. 11). Fixed another cast: strlen expects const char * etc. 12). Fixes uninitialized variable keystroke in nwipe_gui_verify function. 13). Fixes uninitialized variable keystroke in nwipe_gui_noblank function 14). Fixes implicit declaration of function 'fileno' in logging.c 15). Markup changes to README.md 16). Added required packages to README.md Oct 3, 2018

PartialVolume added some commits Oct 3, 2018

Fixes spurious character at the end of serial no string.
Reads the raw 20 character string in a 21byte null terminated character array.
@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Oct 7, 2018

@martijnvanbrummelen I've resolved those conflicts.

I'm assuming your doing a merge rebase, or are you making changes manually?

One conflict would require you to merge ef24557 as two separate merges alter the same line in different ways and the merge above is the latest.

I did notice that you had a free(d) where there was no corresponding mallocs in the function. Line 288 in pass.c

screenshot_20181008-000706

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Oct 8, 2018

One thing I have learnt is it's better to put these fixes into separate branches before doing a pull request. I would then have a far less busy pull request. Live & learn I guess.

@martijnvanbrummelen I'm going to list the patches merged so far (8 off I believe) and those waiting for review/merge. So I can see how far before this pull request gets closed. I the meantime I won't add any further patches until this pull request is clear so I can get my master sync'ed with yours.

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Oct 10, 2018

Hi Martijn,

I have a question, instead of copying my commits from this very busy pull request, if you don't want to simply merge the whole pull request, why don't you cherry pick the individual commits as described here https://mattstauffer.com/blog/how-to-merge-only-specific-commits-from-a-pull-request/ ?. I would imagine it's much faster for you than retyping my commits, less error prone as you may enter typos and it also removes the confusion from my end where you are creating a commit that then causes a conflict in the pull request with my identical commit which I then have to fix.

For my part I appreciate I should have created each fix in a separate branch, hence producing multiple pull requests rather than this large pull request full of separate fixes.

Hope you don't mind my observations. Learning git as I go along. Any comments most welcome.

Nick

@martijnvanbrummelen

This comment has been minimized.

Copy link
Owner

martijnvanbrummelen commented Oct 11, 2018

I prefer separate commit for each issue, instead of one big chuck of merge request.
This makes it easier to test and verify code instead of the big chuck which fixes all and most likely breaks something.
I really appreciate your work and effort keep it up!

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Oct 11, 2018

@martijnvanbrummelen Thanks, much appreciated. I thought I'd start making a list of commits that have been merged as there is quite a few to go yet. This helps me keep track of where we are so far.

Partial "merge", current status.

>>> Merged by MvB <<<
PartialVolume@e38c19a rewritten by MvB as dd74c42 (Fix segfault by initialising nwipe_gui_thread)
PartialVolume@2a44c2a rewritten by MvB as a9ed8a0 (Memory leaks in pass.c)
PartialVolume@65e3d7e rewritten by MvB as e16da66 (memory leaks, s & t)
PartialVolume@b035b8f rewritten by MvB as 5912d0f (check correct pointer)
PartialVolume@4dcd7aa rewritten by MvB as 0ecc907 (Fix cast)
PartialVolume@ef6963a rewritten by MvB as PartialVolume@4dcd7aa (fix cast/serial no)
e4642e0 rewritten by MvB as 81850bd (fix unitialised variable in nwipe_gui_noblank)
Rewritten by MvB 050638b Fix footer information in methods screen

>>> Waiting to be merged by MvB <<<
7b7b9eb Fix non display of serial numbers for ATA/SCSI devices
ef24557 Fix serial no - correct the way serial no. is copied
6c88427 fix unitialised variable in nwipe_gui_verify
d3d3aa2 Fixes segfault by initialising nwipe_misc_thread_data.nwipe_selected
facbbe4 Fixes implicit declaration of function fileio
b1dcd92 Remove /bits/sigset.h if present in nwipe.c

I believe this is a complete list of the remaining fixes to be merged

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Oct 13, 2018

@martijnvanbrummelen

I've gone through the pull request and I believe this is a list of the remaining bug fixes waiting to be merged. I've not included commits regarding the readme files as I will probably redo those as a single commit in a new branch & pull request. This list should be a lot clearer than reading the commits from the pull request.

>>> Waiting to be merged by MvB <<<
7b7b9eb Fix non display of serial numbers for ATA/SCSI devices
ef24557 Fix serial no - correct the way serial no. is copied
6c88427 fix unitialised variable in nwipe_gui_verify
d3d3aa2 Fixes segfault by initialising nwipe_misc_thread_data.nwipe_selected
facbbe4 Fixes implicit declaration of function fileio
b1dcd92 Remove /bits/sigset.h if present in nwipe.c

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Oct 17, 2018

@martijnvanbrummelen

Hi, don't commit those remaining commits yet. I'm going to use this opportunity to improve my git skills.

I'm going to cherry pick those commits and put each in a separate branch, squash any that are multiple commits for the same fix so each fix is a separate fix and cleanup my master.

Once I've figured out what I'm doing I'll close the existing pull request and open 5 new pull requests, each one being for a single fix or enhancement.

That should make it significantly quicker for you to then just click the merge button rather than retyping the commits which are most likely going to get a whole lot more complex as new features are added.

@martijnvanbrummelen

This comment has been minimized.

Copy link
Owner

martijnvanbrummelen commented Oct 17, 2018

sounds good!

@PartialVolume

This comment has been minimized.

Copy link
Contributor Author

PartialVolume commented Oct 23, 2018

Closing pull request to issue commits as separate pull requests.

@PartialVolume PartialVolume deleted the PartialVolume:master branch Nov 2, 2018

@PartialVolume PartialVolume restored the PartialVolume:master branch Nov 2, 2018

@PartialVolume PartialVolume deleted the PartialVolume:master branch Nov 3, 2018

@martijnvanbrummelen

This comment has been minimized.

Copy link
Owner

martijnvanbrummelen commented on e38c19a Nov 16, 2018

committed

@martijnvanbrummelen

This comment has been minimized.

Copy link
Owner

martijnvanbrummelen commented on e4642e0 Nov 16, 2018

committed

@martijnvanbrummelen

This comment has been minimized.

Copy link
Owner

martijnvanbrummelen commented on ef24557 Nov 16, 2018

committed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment