[gnuastro-devel] [bug #46241] ImgCrop segfaults with long output file names

[Mohammad Akhlaghi]

Follow-up Comment #3, bug #46241 (project gnuastro):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?46241>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/