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

Fix signal handlers and memory related bugs #26

Merged
merged 4 commits into from
Dec 1, 2017

Conversation

tansly
Copy link
Contributor

@tansly tansly commented Nov 26, 2017

By running the program in Valgrind and inspecting the code visually, I have uncovered some memory related bugs. I belive this set of commits fixes those bugs. These bugs were:

  • Memory leak on screen resize. The matrix was not properly freed in the var_init() function, so each time the screen is resized, some amount of memory was leaked.
  • Uninitialized use of matrix attribute bold. The bold attribute of the matrix cells was not initialized anywhere in the code. I initialized them to 0 in var_init(). Although I'm not sure what the default value should be, some value must be set somewhere.
  • Wrong sizeof in matrix allocation. The Matrix, which contains pointers to rows of struct cmatrix, was allocated with sizeof(cmatrix), as matrix = nmalloc(sizeof(cmatrix) * (LINES + 1));. This did not cause a problem because, by coincidence, the struct contained two ints which in total have the size of a pointer. I fixed that allocation, and also improved the allocation strategy by allocating all the rows at once, making the free'ing logic simpler and more efficient.

Also, I've noticed that the signal handlers called non signal-safe functions, which poses some serious problems if multiple SIGWINCHes arrived within short intervals. To observe the problematic behaviour, one can resize the terminal multiple times very quickly. This generally leads to a segmentation fault. I fixed this by making the signal handler set a global variable to indicate the signal, and checking this variable at each iteration of the main loop and taking the appropriate action.

Fixes the wrong sizeof in matrix allocation.
Also improves the allocation strategy for the matrix,
it allocates one contiguous array instead of allocating row by row.
Properly free the matrix on reallocation.
Fixes the memory leak on screen size change.
Fixes uninitialized use of the bold attribute in the matrix
by setting a default value of 0 in var_init().
The signal handlers were not safe.
This commit moves the handling logic outside the handler functions and
makes the handler set a global variable to indicate caught signals.
Copy link
Owner

@abishekvashok abishekvashok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work

@abishekvashok abishekvashok merged commit f52a93e into abishekvashok:master Dec 1, 2017
@abishekvashok
Copy link
Owner

Thanks @tansly

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

Successfully merging this pull request may close these issues.

2 participants