bugGNU Astronomy Utilities - Bugs: bug #52544, Incorrect sanity check in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #52544: Incorrect sanity check in lib/data.c

Submitter:  Vladimir Markelov <vvm>
Submitted:  Tue 28 Nov 2017 05:37:24 AM UTC
   
 
Category:  Libraries Severity:  2 - Minor
Item Group:  Source code Status:  Fixed
Privacy:  Public Assigned to:  vvm
Open/Closed:  Closed

Sun 03 Dec 2017 03:10:28 AM UTC, comment #3: 

The fix has been merged with master and pushed to the main repo.

Thanks a lot Vladimir.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sun 03 Dec 2017 12:37:24 AM UTC, comment #2: 

I've created a pull request in my repository:
https://github.com/VladimirMarkelov/gnuastro-vvm/tree/data-sanity-check

The commit:
https://github.com/VladimirMarkelov/gnuastro-vvm/commit/37df78c50837d8b1184525f94ab45bd893a5ed80

Changes:
1. Old check for <=0 was replaced with == check (the message was fixed as well)
2. Added check for possible overflow while multiplying (for cases ndim > 1)
3. Added a warning if there is no overflow but the size is too big

Vladimir Markelov <vvm>
Tue 28 Nov 2017 11:31:12 AM UTC, comment #1: 

Thanks a lot Vladimir, very good point! This had slipped through my fingers and I am really happy your precision in finding such issues helped us find it.

We don't want a dimension to have a value of zero or negative, but as you rightfully mentioned, `dsize[]' has a type of `size_t'. So I suggest the following:

1. To convert the `<=0' check to `==0' and correct the error message to only mention a zero-length dimension.

2. For cases where the user has (mistakenly) given a negative value, we can do a check like what you mentioned (with `SOME_LIMIT'). However, I feel that it would be more robust if we just print a warning (to `stderr') instead of aborting with an error. If the value is indeed negative, later processing steps (for example allocation) should breakdown and abort the program. About the value of SOME_LIMIT, how about using half the maximum value of `size_t' on the system (32-bit or 64-bit)?

What do you think? If this is ok, since you found the issue, can you implement it so I merge it into master?

Thanks again for so nicely detecting such issues :-)

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 28 Nov 2017 05:37:24 AM UTC, original submission:  

Starting from line 325 in lib/data.c

  if(dsize[i]<=0)
    error(EXIT_FAILURE, 0, "%s: the size of a dimension cannot be "
          "zero or negative. dsize[%zu], but has a value of %zu",
          __func__, i, dsize[i]);


dsize is size_t that is unsigned type and can never be negative. So, either we change to 'if(dsize[i]==0)' and fix the error message, or we should think of too big value and change to 'if (dsize[i] == 0 || dsize[i] > SOME_LIMIT)' and fix the error message like '%s: the size of dimension must be between 1 and SOME_LIMIT, but ....'.

It is an easy fix but I am not sure about high limit value.

Vladimir Markelov <vvm>

 

(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 (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-03 makhlaghi StatusNone Fixed
        Open/ClosedOpen Closed
    2017-11-28 makhlaghi Assigned toNone vvm

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code