jasper-software / jasper

Official Repository for the JasPer Image Coding Toolkit
http://www.ece.uvic.ca/~mdadams/jasper
Other
223 stars 101 forks source link

Don't init twice in jas_cm.c #267

Closed jubalh closed 3 years ago

mdadams commented 3 years ago

@jubalh This code is not portable, as the C standard does not require that the null pointer be represented literally as all zero bits. The memset function will set everything to all zero bits, however. While this will work on many systems, it is not guaranteed to work on all systems on which JasPer might be used. This is why the memset call is not good enough to initialize the members that are pointers. They must be set to null. The literal pointer value of 0 is automatically mapped by the C language implementation to whatever bit pattern is used for null pointers. So assigning "0" to a pointer is portable, but memsetting a pointer to 0 is not.

MaxKellermann commented 3 years ago

If JasPer's portability requirements don't allow this, then memset() should be avoided completely. Yet another reason to use C++ instead of C, which, unlike C, has sane initialization semantics ...

mdadams commented 3 years ago

As far as language standards are concerned, neither C nor C++ require that null pointers are represented with all zero bits. Of course, many C/C++ implementations do represent null pointers as all zero bits. Aside from some segmented architectures, there are probably not too many systems out there for which null pointers are not actually represented with all zero bits. It's just that JasPer is used on many platforms, and I would not want to assume something about the representation of null pointers that is not guaranteed by the language standard.

All of this said, zeroing memory with memset does have the benefit of making bugs more deterministic (at the cost of potential redundant initialization). The act of zeroing the bytes of a pointer in memory by itself should never cause a problem (as long as the pointer value is initialized again before its use). It's just that zeroing memory is not guaranteed to be setting any pointers in that memory to the null value. So, memset should not be relied on to initialize pointers to the null value if portability is an important consideration.

As for C++ versus C, I would tend to agree with you there. I have only used C++ for new projects in recent memory. C++ has come a long way since circa 1999 when JasPer was first developed.

MaxKellermann commented 3 years ago

My point was not that nullptr in C++ needs to be all-zeroes; C++ is better at initialization because you can easily declare structs in a way that they are always guaranteed to be properly initialized. In C, every code location which instantiates a struct needs to repeat that, detached from the actual struct declaration, which is error prone. Everything about C is error prone and cumbersome. Many of JasPer's past bugs wouldn't have been possible in C++.

And with C++, you could probably cut JasPer's source code to less than half, with less bugs and more performance. If JasPer were a relevant project today, an (incremental) switch to C++ would be very worthwile.

All of this said, zeroing memory with memset does have the benefit of making bugs more deterministic

I disagree. All this does is hide initialization bugs from tools like valgrind. This actually prevents finding bugs.

Better initialize all variables explicitly with useful values. Zeroing bits is only sometimes a useful value.