There are a couple of categories of unsafe functions that are used throughout VTK/ParaView/kwsys and third-party libraries. For the third-party libraries we can’t do anything, but for VTK/ParaView/kwsys we should replace them with safer alternatives. The discussion about this topic was initiated in the ParaView issue.
The unsafe C functions categories that should be replaced are the following
ato*, e.g. atoi, atof e.t.c.
mem*, e.g. memcmp/memcpy/memset e.t.c.
str*. e.g. strcpy, strcmp, strcat, strlen, strtok e.t.c.
To implement these changes in the existing code base, we need to decide which project will include fmt and/or safestringlib. Since ParaView inherits everything from VTK, we don’t need to add anything to it. As of now VTK has fmt as a third-party library.
Based on my understanding we have the following options:
Add safestringlib to VTK and let kwsys as it is (with the unsafe functions)
Add safestrinblib to kwsys and let kwsys’s printf/sprintf/fprintf.snprintf functions be. This way VTK will inherit safestringlib functionality
Move fmt to kwsys and add safestringlib too to kwsys. This way VTK will inherit fmt and safestringlib functionality
Any suggestions or comments would be more than welcome!
memcmp and memset seem fine to me; they have explicit bounds passed in. memcpy has the standard buffer juggling issues though.
std::string and fmt should handle all of the use cases here.
I don’t think the half-way things here are any use; just use std::string (or std::vector<char> for raw bytes) and fmt instead. I don’t think safestringlib is of any interest to C++ codebases.
While true, I’d rather things be correct and then pointed out “this needs to be faster” than the reverse after a debugging session.
Note there are multiple pitfalls with memcpy:
it calculates in bytes, not objects (overflow on the manual multiplication is…a thing)
only works on POD types with trivial constructors (other types do not “exist” without their constructor being called; custom copy ctors may also block the destination from being valid objects)
does not verify that the source and destination types are compatible
So after some discussion of how this can go, some things that came up:
Compatibility
So vtkStdString is on my list of things to go, but we can use its drawback to help bridge the compatibility here. It has an implicit const char* conversion operator (very dangerous, but no worse than C pointers anyways). We can use this to our advantage here. Note that classes that use nullptr as some magic value would need something like vtkOptionalString with convenience methods to convert to nullptr if it is none. Converting a char* member to vtkStdString can be kept compatible with:
void SetMember(const char* _arg) { // Need to handle `nullptr` callers
if (_arg) {
this->SetMember(vtkStdString(_arg));
}
}
void SetMember(vtkStdString _arg); // impl in C++ code (see below)
vtkStdString const& GetMember() const { return this->Member; }
vtkStdString Member;
// --------------
void SetMember(vtkStdString _arg) {
if (_arg != this->Member) {
this->Member = std::move(_arg);
this->Modified();
}
}
nullptr is valid:
void SetMember(vtkOptionalString _arg); // impl in C++ code (same body as above)
vtkOptionalString const& GetMember() const { return this->Member; }
vtkOptionalString would be another interim class to help bridge the gap here which just turns nullptr into std::none_t. It’s implicit const char* conversion would similarly be deprecated someday (just like vtkStdString's).
vtkSetGet macros
I find these macros to be more noise than useful. About the only thing they do is:
avoid typos
ensure Modified() is called properly in setters
Other than that, I see no benefit. I think that unit tests are far better for ensuring such things. The benefits:
virtual would no longer be used for these things
getters could be const
getters could return const& instead of forcing a copy
setters could std::move for non-POD types (allowing the caller to decide if memory is reused instead of pessimizing and forcing a copy even when the caller doesn’t need the string anymore)
headers lose a lot of unnecessary code that triggers a full rebuild when vtkSetGet is changed
the member name is no longer pinned to the method name (allowing for more PIMPL usage)
The inline-ness is largely useless for the setters because VTK doesn’t support devirtualization of Modified calls (the use of factory constructors hides necessary knowledge even for vtkNew<vtkObject> obj; obj->SetSomething();), so the code is the same for every single caller and any actual inlining is of no benefit (it just inflates the caller’s code size).
Existing callers can pass nullptr; we need to guard against that.
why do you need to do an implicit conversion
So that the member can be automatically managed instead of being manual (with macro support)
Why use vtkStdString at all?
Because we need to preserve const char* access to the getter (it is currently char*, but actually modifying it bypasses Modified() calls to everyone’s great enjoyment I’m sure; anyone actually doing that is just setting themselves up for failure). Making the return type vtkStdString const& preserves that conversion (auto* s = obj->GetString(); is messed up though…but there’s an easy compatible way to write it too with an explicit const char*).
Because char* management is finicky. Common faults:
not releasing the buffer before reuse
using the wrong allocation mechanism (vtkSetGet uses new[], so delete[] is needed)
forgetting to set it back to nullptr in the destructor
In order to remove our use of C string library functions, not using char* for strings is basically required unless we’re going to pay for a strlen per string_view constructed from one for further manipulations. You may as well just bundle the allocation and length memorization in std::string (of which, vtkStdString is a better compatibility story).
I think that this work needs to be split into two parts.
Replace:
ato*, e.g. atoi, atof e.t.c.
printf, sprintf, snprintf
fprintf, fwrite
scanf
fscanf
Replace
mem*, e.g. memcmp/memcpy/memset e.t.c.
str*. e.g. strcpy, strcmp, strcat, strlen, strtok e.t.c.
The first part will fix half the safety issues and give us greater performance in all vtk writers.
The second part will help us fix the remaining safety issues and completely remove the char* usages, after figuring out the best cost/effective approach (which we are close to do so).
The priority here is to remove unsafe C functions. If vtkStdString can help us minimize that cost. And later on, just do using vtkStdString = std::string, it would be ideal.
Sure, but the inconsistency of “some members use Get prefixes, others don’t” would persist without another round of deprecation and renaming on everyone’s plate.
vtkStdString itself wouldn’t change; it provides a nice ramp between the status quo and a better world (without either naming inconsistencies or double renames in the long run).
We should also ban all uses of functions in this manpage (the is* family of ASCII classification functions) because they are UB if the argument is not in [0,127] or EOF itself.