bugGNU 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: 

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:

  • Copy one of the existing tests in the 'tests/progname/' to a new script (with a clear name).


  • Change the last line or two (the actual command that is to be tested) to implement the feature you want to test. All the test scripts are thoroughly commented, please keep the comments.


  • Add the test script address to the 'TESTS' variable in 'tests/Makefile.am'


  • Specify the script's dependency(s) in the dependency structure of 'tests/Makefile.am'. The comment explains the logic behind it.


  • Run 'autoreconf -f' which will make 'tests/Makefile.in' so the next configure script builds the proper Makefile to run the test.
Mohammad Akhlaghi <makhlaghi>
Group administrator
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.

Another important, but sometimes missed, purpose of a test suite is to prevent fixed bugs to come back later again during development.  I suggest to add a test for all recently fixed bugs, when possible.

For example, for the current bug a possible test (to be adapted) is given in the original report of the issue:


src/imgcrop/astimgcrop --xc=50 --yc=50 tests/1_imgcat.fits --output=qwertyuiopasdfghjklzxcvbnmQWERTYUIOPASDFGHJKLZXCVBNM1234567890.fits


Something like this (a very long output file name) can be actually used for the imgcrop test (actually this is bound to the optimization flag set during configuration, it isn't something that can be always tested, but -O3 is the default).

Mosè Giordano <giordano>
Group Member
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>
Group administrator
Wed 03 Aug 2016 11:53:34 PM UTC, comment #6: 

Great! It has been merged to master and pushed to the main repo.

I made some further corrections that I have fully explained in the commit message: 1) Added a macro to remove the usage of 30 (and 27) in the code. 2) renamed the variable. 3) fixed some other GNU Coding Standards issues related to indentation. Please see  the next commit message for a more complete explanation.

Mohammad Akhlaghi <makhlaghi>
Group administrator
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.

Branch
Patch

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.

So when you need to add new variables, I would be greatful if you could add them in the start of the function along with all the rest of the variables (possibly in the same line as other variables of the same type).

Mohammad Akhlaghi <makhlaghi>
Group administrator
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.

I also think the updated solution (without dynamic allocation) is better. These macros are set to make a readable and easy to follow output on the terminal for the user. They have no other purpose.

If a user needs to confirm the status of one of the crops thoroughly, they should use the log file which is another output of ImageCrop ('astimgcrop.log'). Not the terminal output. There are no macros there and any file name with a theoretically unlimited length can be printed there.

So I also believe your last solution is the best fix. The updated code in the next comment also seems to be much more elegant than dynamic allocation because the length is fixed: we just have to define a macro for the 30.

I just had a comment: if the last 27 characters of the long file name (including its '.fits' suffix) are printed out (instead of the first 30 characters) followed by a '...' it is much more familiar to the eye and clearly show that the truncation. For example the current solution will print:


ImageCrop started on Tue Aug  2 13:17:51 2016
  - Read metadata of 1 images.                    0.001170 seconds
  ---- qwertyuiopasdfghjklzxcvbnmQWER 1 1
ImageCrop finished in:  0.004010 seconds


But if it is in the following format it would be much better.


ImageCrop started on Tue Aug  2 13:17:51 2016
  - Read metadata of 1 images.                    0.001170 seconds
  ---- ...JKLZXCVBNM1234567890.fits 1 1
ImageCrop finished in:  0.004010 seconds


Especially, since long file names are probably created by a large number of directories prefixed to a (possibly) short filename. This is an easy correction and if you don't have the time to implement it I am happy to do it after merging your commit.

I also had two notes about the commit:

  • Your very complete explanation here on Savannah was great. It would be great if you could include this thorough explanation you gave here (elaborating the cause and listing all the possible solutions) in the commit message also. In the future, you can keep such great explanations only for the commit message and just link to it here in Savannah. In general, it is much better to do all the explanations in the commit message and only put a link to the commit here. The commit message is kept as part of the project inside the project history, so it is much more easier for everyone to check (for example when they are offline) compared to this webpage.


  • In the code, I would be greatful if you could follow the GNU Style of indentation, in particular giving a full line to the opening and closing curly braces. For example the code on the commit in Github should be like:



if (strlen(log->name) > 30)
  {
    char *short_name = calloc(31, sizeof(char));
    strncpy(short_name, log->name, 30);
    sprintf(msg, "%-30s %lu %d", short_name, log->numimg,
            log->centerfilled);
    free(short_name);
  }
else
  sprintf(msg, "%-30s %lu %d", log->name, log->numimg,
          log->centerfilled);


(I also removed the curly braces after 'else' because there is only one expression and they aren't needed ;-)).

Thanks again for the solution, I look forward to merging with master once these minor issues are corrected ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
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):

char job_name[31];
strncpy(job_name, log->name, 30);
job_name[30] = '\0';
sprintf(msg, "%-30s %lu %d", job_name, log->numimg,
                  log->centerfilled);
gal_timing_report(NULL, msg, 2);

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.

Branch

Patch

=== Why I fixed it that way ===
I see a few ways to fix the memory corruption at this point:
1. Increase GAL_TIMING_VERB_MSG_LENGTH_V
If we do this we have to change a few other constants (GAL_TIMING_VERB_MSG_LENGTH_T, GAL_TIMING_VERB_MSG_LENGTHS_2_V etc). It can results in a lot of code changes - easy way to break something. Moreover, we cannot say what will be the maximum file name length, so we have to reserve decent amount of memory(hundreds of bytes at least)

2. Do not use GAL_TIMING_VERB_MSG_LENGTH_V for buffer size and choose bigger number.
Similar to item 1: we do not know for sure the maximum file name length. But it does not require to change other variables.

3. Do not use static char buffer for the msg and allocate memory dynamically
It is fine for any file name length. The only drawback: I see format string uses '%-30', so if we use longer strings the output will look ugly because formatting won't work.

4. Truncate file name to 30 characters
It works with any file name and it keeps log output nice-looking because format string '%-30' works always here.


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]);".

Recipe to reproduce the bug: run


src/imgcrop/astimgcrop --xc=50 --yc=50 tests/1_imgcat.fits --output=qwertyuiopasdfghjklzxcvbnmQWERTYUIOPASDFGHJKLZXCVBNM1234567890.fits


Mosè Giordano <giordano>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by makhlaghi (Posted a comment)
  • -email is unavailable- added by vvm (Posted a comment)
  • -email is unavailable- added by giordano (Submitted the item)
  •  

    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.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-03 makhlaghi StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-07-31 vvm StatusNone In Progress
        Assigned toNone vvm

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code