GNU Astronomy Utilities - Bugs: bug #46241, ImgCrop segfaults with long output...
You are not allowed to post comments on this tracker with your current authentication level.
bug #46241: ImgCrop segfaults with long output file names
Submitter: | Mosè Giordano <giordano> | ||
Submitted: | Sun 18 Oct 2015 04:54:44 PM UTC | ||
Category: | Crop | Severity: | 3 - Normal |
Item Group: | Crash | Status: | Fixed |
Privacy: | Public | Assigned to: | vvm |
Open/Closed: | Closed |
Jump to the original submission
Fri 05 Aug 2016 11:03:14 PM UTC, comment #9: |
Mohammad Akhlaghi <makhlaghi>![]() |
Fri 05 Aug 2016 10:32:53 PM UTC, comment #8: The main point of having a test suite is to make sure that numerical results are always correct (or at least what is currently supposed to be correct) also after a massive change.
|
Mosè Giordano <giordano>![]() |
Fri 05 Aug 2016 07:13:14 PM UTC, comment #7: While doing some further testing, I noticed that the fix was only for ImageCrop's image mode. The WCS mode would still crash when given a long file name. So I moved the fix inside a separate function which is now called by both image and WCS modes. The change has been pushed to master. |
Mohammad Akhlaghi <makhlaghi>![]() |
Wed 03 Aug 2016 11:53:34 PM UTC, comment #6: Great! It has been merged to master and pushed to the main repo.
|
Mohammad Akhlaghi <makhlaghi>![]() |
Wed 03 Aug 2016 07:55:39 PM UTC, comment #5: I agree that it is better to show the end of the job name. I thought of, too. So, I created a simpler solution without extra string copying and with displaying the end of the job name instead of its beginning. Please see it in separate branch (I have not removed my branch with old draft patch yet, so be careful :) ). It is the final one.
|
Vladimir Markelov <vvm>![]() |
Tue 02 Aug 2016 11:52:33 AM UTC, comment #4: I just wanted to add that although variable definitions (like 'char job_name[31];' can be done inside a block, until now in Gnuastro, we have followed the style that all variables are defined at the start of the function.
|
Mohammad Akhlaghi <makhlaghi>![]() |
Tue 02 Aug 2016 11:44:51 AM UTC, comment #3: Great solution Vladimir, thank you. I tested it and it is indeed the correct solution to the problem.
|
Mohammad Akhlaghi <makhlaghi>![]() |
Mon 01 Aug 2016 08:29:45 PM UTC, comment #2: The patch was not final, it was more like a draft. I just wanted to show the way to fix. I like more the patch without dynamic memory allocation (example):
If my fix is accepted, I'll rewrite my patch in the way above and then it can be merged to the master branch |
Vladimir Markelov <vvm>![]() |
Sun 31 Jul 2016 11:54:25 PM UTC, comment #1: I found a place where application memory corrupts and makes the application fail. It may fix bug 45380 as well.
|
Vladimir Markelov <vvm>![]() |
Sun 18 Oct 2015 04:54:44 PM UTC, original submission:
Somewhat related to bug #45380. When compiled with optimization flag -O2 or higher, if the output file names is longer than ~50 characters ImageCrop segfaults when trying to free "crp". "crp" changes its values after calling "imgmodecrop(&crp[0]);", you can see it with gdb or printf'ing the value of "crp" before and after calling "imgmodecrop(&crp[0]);".
|
Mosè Giordano <giordano>![]() |
No files currently attached
Depends on the following items: None found
Items that depend on this one: None found
There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.
I completely agree. One of the main reasons ConvertType converts FITS images to text files was ultimately to use it in numeric tests. For example we could cut the central 3 or 5 pixels of the images to see if they were properly made/processed or not (by comparing it with a reference text 3x3 or 5x5 ascii file in the source).
You are also right about using tests for making sure that bugs do not re-surface. Infact it is also advised in the book.
But unfortunately I have been too busy to implement these features my self. It would be great if the bug fix commit could also come with a test. Adding tests is really easy: