Re: warnings originating in gl2ps -- update

[John W. Eaton]

On 11-Feb-2011, CdeMills wrote:

| > Enclosed. I tried 'hg export', but the patches are only between
| > release-3-4-x and tip. I produced the patches with 'hg diff'. Seven files
| > touched, including the Changelog. All warnings gone.
| > 
| 
| Sorry, the fix in util.cc was wrong. Strangelly,
|  if (nchars > -1 && size - nchars > 0)
| makes around half of the tests to fail!
| 
| The right fix is to keep
|    if (nchars > -1 && nchars < size)
| and declare size as a ssize_t instead of a size_t. With this change, there
| aren't any test failures.

| diff -r 357d593d87c1 src/load-path.cc
| --- a/src/load-path.cc        Thu Feb 10 00:58:31 2011 -0500
| +++ b/src/load-path.cc        Fri Feb 11 12:11:22 2011 +0100
| @@ -617,7 +617,7 @@
|    while (k > 1 && file_ops::is_dir_sep (dir[k-1]))
|      k--;
|  
| -  if (k < dir.length ())
| +  if (dir.length () - k > 0)
|      dir.resize (k);
|  
|    return dir;

Looking at this one, I don't think any change is needed except to
declare K to be size_t instead of octave_idx_type since we don't need
an octave_idx_type value in this function.  I'll make this change.


| diff -r 357d593d87c1 src/mex.cc
| --- a/src/mex.cc      Thu Feb 10 00:58:31 2011 -0500
| +++ b/src/mex.cc      Fri Feb 11 12:11:22 2011 +0100
| @@ -1165,7 +1165,6 @@
|        cpr[i] = str[i];
|    }
|  
| -  // FIXME??
|    mxArray_number (mwSize m, const char **str)
|      : mxArray_matlab (mxCHAR_CLASS, m, max_str_len (m, str)),
|        pr (calloc (get_number_of_elements (), get_element_size ())),
| @@ -1186,7 +1185,7 @@
|          for (size_t i = 0; i < tmp_len; i++)
|            cpr[m*i+j] = static_cast<mxChar> (ptr[i]);
|  
| -        for (size_t i = tmp_len; i < nc; i++)
| +        for (ssize_t i = tmp_len; i < nc; i++)
|            cpr[m*i+j] = static_cast<mxChar> (' ');
|        }
|    }

Let's take a look at this function:

  mxArray_number (mwSize m, const char **str)
    : mxArray_matlab (mxCHAR_CLASS, m, max_str_len (m, str)),
      pr (calloc (get_number_of_elements (), get_element_size ())),
      pi (0)
  {
    mxChar *cpr = static_cast<mxChar *> (pr);

    mwSize *dv = get_dimensions ();

    mwSize nc = dv[1];

    for (mwIndex j = 0; j < m; j++)
      {
        const char *ptr = str[j];

        size_t tmp_len = strlen (ptr);

        for (size_t i = 0; i < tmp_len; i++)
          cpr[m*i+j] = static_cast<mxChar> (ptr[i]);

        for (size_t i = tmp_len; i < nc; i++)
          cpr[m*i+j] = static_cast<mxChar> (' ');
      }
  }

Suppose we are compiling Octave for a system with 32-bit pointers
where size_t is an unsigned 32-bit integer.  We also currently have
mwSize defined to be octave_idx_type, which would be a 32-bit signed
integer.  Then we have

  static mwSize
  max_str_len (mwSize m, const char **str)
  {
    int max_len = 0;

    for (mwSize i = 0; i < m; i++)
      {
        mwSize tmp = strlen (str[i]);

        if (tmp > max_len)
          max_len = tmp;
      }

    return max_len;
  }

so it is already possible (though not likely) for one of the strings
to have a length that can't be stored in mwSize, so max_str_len could
return 0 instead of something larger than the maximum value that can
be stored in a 32-bit signed integer.  Of course, trying to create a
character string this large won't work well in Octave anyway, because
it will take up more than half of the available memory, and we are
creating a copy of it here, so that will fail before we even get to
the line that gives the warning.

Now consider the situation on a system with 64-bit pointers with
size_t being a 64-bit unsigned integer, but compiled to use a 32-bit
signed integer for octave_idx_type (and mwSize).  In that case, it
could be possible to create two (or more) copies of a character string
with more than 2^31 elements, but the max_str_len function could still
return 0 (or at least fail to do the right thing if STR points to more
than one string).  So then we could have something like

  str[0] = "a short string"
  str[1] = string with more than 2^31 characters

then strlen (str[i]) would return the correct counts in both cases,
but storing the large result in TMP would give a negative value, so
MAX_LEN would actually be the length of the shorter string.  Then back
in the mxArray_number constructor, PR will not be allocated large
enough and we're in trouble.  Changing the declaration I in the second
nested loop to be ssize_t won't fix that problem.

Perhaps I'm missing something, but I think that we should examine all
of these cases more carefully rather than just rearrange comparison
expressions to avoid warnings.

And I still think we should consider changing octave_idx_type to be
unsigned, except that currently we use it for things like DIM
arguments where a value of -1 means "use the default".  So we would
need another method for things like that.  But if octave_idx_type were
signed, then we would still need a signed type to use for integer
values that are passed to Fortran subroutines.  But then we could just
do the necessary checks at the point where we call Fortran code, and
the rest of Octave might be in better shape regarding array indexing
for very large arrays since it could be using unsigned size_t
everywhere else.

But in any case, I think there can only really be a problem if we are
using 32-bit integers for indexing for Fortran on systems with 64-bit
pointers, because otherwise there is no way to create more than one
array large enough to cause trouble, and given the way Octave works,
you nearly always end up needing to create a few temporary copies of
an array just to do anything with it.  So you would be out of memory
before you noticed the problem with integer values overflowing and
wrapping around to negative values.

jwe