On 3/27/23 11:03, Eelco Chaudron wrote: > > > On 16 Mar 2023, at 17:36, James Raphael Tiovalen wrote: > >> This commit addresses several high and medium-impact Coverity defects by >> fixing several possible null-pointer dereferences and potentially >> uninitialized variables. >> >> There were cases when crashes were encountered when some null pointers >> were dereferenced. Null pointer checks and alternative code paths have >> been added to prevent unwanted crashes. Additionally, some struct >> variables were not initialized. Zero-initializations have been added to >> prevent potential data leaks or undefined behavior. >> >> Some variables would always be initialized by either the code flow or >> the compiler and some pointers would never be null. Thus, some Coverity >> reports might be false positives. That said, it is still considered best >> practice to perform null pointer checks and initialize variables upfront >> just in case to ensure the overall resilience and security of OVS, as >> long as they do not impact performance-critical code. As a bonus, it >> would also make static analyzer tools, such as Coverity, happy. >> >> Unit tests have been successfully executed via `make check` and they >> successfully passed. > > I did not review this, but I noticed you do null checks for memset() and > memcpy(). But there are functions for this in OVS like nullable_memset() and > nullable_memcpy(), maybe some of the changes might benefit from using this. > > In addition, there is also a nullable_string_is_equal() which might be useful > in some cases.
I'd also say that the vast majority of changes here should be replaced with assertions, because they are checking for impossible conditions. For example, there should be no way the name and tables are not set after successful parser run in ovsdb_schema_from_json(), because they are not optional. Explicit handling of impossible cases is confusing for readers. Use of assertions will also reduce the patch size significantly. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
