warning: null destination pointer

[1Hyena]

When compiling a project that uses CImg, I get the following warning:

In member function ‘cimg_library::CImgList<T>& cimg_library::CImgList<T>::_load_gif_external(const char*, bool) [with T = unsigned char]’,
    inlined from ‘cimg_library::CImgList<T>& cimg_library::CImgList<T>::load_gif_external(const char*) [with T = unsigned char]’ at /usr/include/CImg.h:64500:30,
    inlined from ‘static cimg_library::CImg<T> cimg_library::CImg<T>::get_load_gif_external(const char*, char, float) [with T = unsigned char]’ at /usr/include/CImg.h:56805:45,
    inlined from ‘cimg_library::CImg<T>& cimg_library::CImg<T>::load_gif_external(const char*, char, float) [with T = unsigned char]’ at /usr/include/CImg.h:56799:64:
/usr/include/CImg.h:64543:29: warning: null destination pointer [-Wformat-truncation=]
64543 |           else cimg_snprintf(filename_tmp2,filename_tmp2._width,"%s-%u.png",filename_tmp._data,i);
      |                             ^

The code from CImg.h around line 64543 is this:

  if (img) { img.move_to(*this); std::remove(filename_tmp2); }
  else { // Try to read animated gif
    unsigned int i = 0;
    for (bool stop_flag = false; !stop_flag; ++i) {
      if (use_graphicsmagick) cimg_snprintf(filename_tmp2,filename_tmp2._width,"%s.png.%u",filename_tmp._data,i);
      else cimg_snprintf(filename_tmp2,filename_tmp2._width,"%s-%u.png",filename_tmp._data,i);
      try { img.load_png(filename_tmp2); }
      catch (CImgException&) { stop_flag = true; }
      if (img) { img.move_to(*this); std::remove(filename_tmp2); }
    }
  }

[dtschump]

I guess this is a false warning: filename_tmp is always allocated here, to a 256-px image, so with a non-null pointer.

[1Hyena]

Actually filename_tmp2 is the destination pointer.

[dtschump]

Yes, both filename_tmp and filename_tmp2 are allocated at the same location (at the beginning of the method), with a size of 256.

[1Hyena]

Ok, seems like the compiler is getting this wrong indeed, but a simply if check would fix this warning too.

      if (img) { img.move_to(*this); std::remove(filename_tmp2); }
      else { // Try to read animated gif
        unsigned int i = 0;
        for (bool stop_flag = false; !stop_flag; ++i) {
          if (use_graphicsmagick) cimg_snprintf(filename_tmp2,filename_tmp2._width,"%s.png.%u",filename_tmp._data,i);
          else if (filename_tmp2) cimg_snprintf(filename_tmp2,filename_tmp2._width,"%s-%u.png",filename_tmp._data,i);
          try { img.load_png(filename_tmp2); }
          catch (CImgException&) { stop_flag = true; }
          if (img) { img.move_to(*this); std::remove(filename_tmp2); }
        }
      }

[dtschump]

True, but to be honest, I'm not in favor of adding unnecessary complexity to the code just to fix a compiler problem. If we have to do that for all compilers, it's endless. That should be reported as an issue to the g++ team.

[1Hyena]

Actually gcc is not giving a false positive warning here. I investigated the constructors of the CImg structure and this is what we're dealing with:

explicit CImg(const unsigned int size_x, const unsigned int size_y=1,
              const unsigned int size_z=1, const unsigned int size_c=1):
  _is_shared(false) {
  const size_t siz = safe_size(size_x,size_y,size_z,size_c);
  if (siz) {
    _width = size_x; _height = size_y; _depth = size_z; _spectrum = size_c;
    try { _data = new T[siz]; } catch (...) {
      _width = _height = _depth = _spectrum = 0; _data = 0;
      throw CImgInstanceException(_cimg_instance
                                  "CImg(): Failed to allocate memory (%s) for image (%u,%u,%u,%u).",
                                  cimg_instance,
                                  cimg::strbuffersize(sizeof(T)*size_x*size_y*size_z*size_c),
                                  size_x,size_y,size_z,size_c);
    }
  } else { _width = _height = _depth = _spectrum = 0; _data = 0; }
}

As you can see, if siz is 0, then _data gets initialized as nullptr and the compiler has a perfectly valid reason to warn about it.

In theory we could of course see that in our case siz would never end up being 0 but you can't expect the compiler to know that. Perhaps if the safe_size method was some sort of constexpr, then it would be possible. I suggest giving this issue a second thought.

[dtschump]

As you can see, if siz is 0, then _data gets initialized as nullptr and the compiler has a perfectly valid reason to warn about it.

I don't agree. All unsigned int arguments of this constructors are passed as const. The variable siz is also const. This means it is *really easy for the compiler to know that an initialization such as CImg<unsigned char> filename(256) generat an instance where siz cannot be zero, hence _data cannot be null (siz can be easily estimated at compile time).

[1Hyena]

If that's supposed to be known compile-time, then you should make it constexpr. That would make your intent explicit.

[dtschump]

It's a different thing : the constexpr specifier declares that it is possible to evaluate the value of the function or variable at compile time. In the case of specifying the arguments of a constructor, it's not always the case (e.g. for siz cannot be always evaluated at compile time). Here, the warning about the null pointer, printed by g++ is clearly abusive. The pointer cannot be null, and it will never be. If g++ is not smart enough to handle such (simple) cases, I think it should not even try to trigger warnings. The CImg code is definitely correct.

[1Hyena]

Ok you have convinced me. Thanks for the clarifications.