1653622 - skICC.cpp gcc warning: the size parameter to strncpy is incorrect.

Status: RESOLVED INVALID

(Core :: Graphics, defect, P3)

Description

[ISHIKAWA, Chiaki]

I obtained the following warning during compilation of mozilla-central
tree source. I was building thunderbird from comm-central tree.

I am using GCC for compilation. Its compile-time warning has become very clever.

/usr/bin/ccache /usr/bin/g++-9 -std=gnu++17 -o Unified_cpp_gfx_skia4.o -c  -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/stl_wrappers -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/system_wrappers -include /NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -DDEBUG=1 -DSKIA_IMPLEMENTATION=1 -DSK_PDF_USE_SFNTLY=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/gfx/skia -I/NEW-SSD/moz-obj-dir/objdir-tb3/gfx/skia -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/gfx/skia/skia -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/gfx/skia/skia/include/third_party/skcms -I/NEW-SSD/NREF-COMM-CENTRAL/mozilla/gfx/sfntly/cpp/src -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/nspr -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /NEW-SSD/moz-obj-dir/objdir-tb3/mozilla-config.h -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wno-multistatement-macros -Wno-error=class-memaccess -Wno-error=deprecated-copy -Wformat -Wformat-overflow=2 -fno-sized-deallocation -fno-aligned-new -fno-builtin-strlen -Wl,--gdb-index -Dfdatasync=fdatasync -DDEBUG_4GB_CHECK -DUSEHELGRIND=1 -DUSEVALGRIND=1 -DDEBUG -g -gsplit-dwarf -Werror=sign-compare -Werror=unused-result -Werror=unused-variable -Werror=format -fuse-ld=gold -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -g -Og -fvar-tracking -gdwarf-4 -fvar-tracking-assignments -freorder-blocks -fno-omit-frame-pointer -funwind-tables -Wno-deprecated-declarations -Wno-overloaded-virtual -Wno-shadow -Wno-sign-compare -Wno-unreachable-code -Wno-unused-function -Wno-logical-op -Wno-maybe-uninitialized -I/NEW-SSD/moz-obj-dir/objdir-tb3/dist/include/cairo -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/uuid -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/fribidi -I/usr/include/harfbuzz -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/uuid -I/usr/include/freetype2 -I/usr/include/libpng16  -MD -MP -MF .deps/Unified_cpp_gfx_skia4.o.pp  -fdiagnostics-color  Unified_cpp_gfx_skia4.cpp
In file included from Unified_cpp_gfx_skia4.cpp:47:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/gfx/skia/skia/src/core/SkICC.cpp: In function ‘void get_color_profile_tag(char*, const skcms_TransferFunction&, const skcms_Matrix3x3&)’:
/NEW-SSD/NREF-COMM-CENTRAL/mozilla/gfx/skia/skia/src/core/SkICC.cpp:278:49: warning: argument to ‘sizeof’ in ‘char* strncpy(char*, const char*, size_t)’ call is the same expression as the source; did you mean to use the size of the destination? [-Wsizeof-pointer-memaccess]
  278 |         strncpy(dst, kDescriptionTagBodyPrefix, sizeof(kDescriptionTagBodyPrefix));
      |                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The relevant line is
https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/core/SkICC.cpp#278

I think GCC warning is correct, and this has to be

      strncpy(dst, kDescriptionTagBodyPrefix, sizeof(dst));

sizeof(dst) would be |kICCDescriptionTagSize| according to
https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/core/SkICC.cpp#268

I doubt this is a security problem, but just in case, I set the
security flag. Please reset it when this is deemed safe.

Comment 1

[Lee Salzman [:lsalzman]]

There isn't any problem here that I can see. sizeof(kDescriptionTagBodyPrefix) is hard-coded to be 12, and that is strictly smaller than sizeof(dst), which is hard-coded to be kICCDescriptionTagSize which is 44. Further, we do not use the Skia image encoder framework which actually uses this code either.

Comment 2

[ISHIKAWA, Chiaki]

OK, the source code is safe as is.

However, getting the warning by GCC-9 as of today, and presumably when clang will become clever as GCC regarding the compile-time check, we will see some similar warning from clang.
And we can't keep track of hard-coded value accidentally/intentionally changed in the future, or who knows we may begin to use the encoder scheme in the future (I am a bit hazy why the code is there if we don't use it.).

So I suggest that we modify the code to the following notwithstanding the comment 1:

strncpy(dst, kDescriptionTagBodyPrefix, sizeof(dst));

Comment 3

[Lee Salzman [:lsalzman]]

We don't own the code. It is difficult getting changes upstreamed to Skia. And if it can't be upstreamed, I have to maintain patches indefinitely. I would suggest filing a bug upstream with them.

In this case, I don't believe the change is justified given the constraints of the code. It just doesn't make sense to change something where the code clearly intends that to be only modifying a subset of the destination buffer (not the whole thing). Sorry.

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Lee Salzman [:lsalzman] from comment #3)

We don't own the code. It is difficult getting changes upstreamed to Skia. And if it can't be upstreamed, I have to maintain patches indefinitely. I would suggest filing a bug upstream with them.

In this case, I don't believe the change is justified given the constraints of the code. It just doesn't make sense to change something where the code clearly intends that to be only modifying a subset of the destination buffer (not the whole thing). Sorry.

I may try talking this with upstream.

only modifying a subset of the destination buffer (not the whole thing).

I was afraid of this, but if that were the case, then the original code ought to have used bcopy or friends, I think. (I am not sure if null filling is that important if only the portion of the target field is filled, but I will talk this with upstream.

Thank you for your time.