Treat appending an empty string as a no-op, not an error (0.6.5) - #36
Merged
Conversation
CMUTIL_StringAddNString folded two unrelated conditions into one branch: a NULL pointer, which is a genuine programming error, and a size of zero, which is not. Appending nothing should quietly do nothing and report the unchanged size. Instead every such call returned -1 and logged an ERROR with a stack trace. The consequence surfaced through Replace. Removing a substring is spelled Replace(needle, ""), and the empty replacement made AddString fail, which sent Replace to its failure path: it destroyed the result and returned NULL. Stripping the dash out of "UTF-8" - a textbook use of Replace - was exactly the combination that broke, and callers that dereferenced the result crashed. XmlSetName has the same shape (Clear then AddString) and logged an ERROR with a stack trace on every empty name, drowning real errors in noise. AddNString now rejects NULL and returns the current size for a zero-size append. AddString, Replace and XmlSetName all follow from that one change. InsertNString carried the same defect and is fixed the same way, with one difference: the index is validated before the size is considered, because an out of bound index stays an error even when there is nothing to insert. InsertString and InsertAnother were missing the NULL guard their Add counterparts already had, and crashed on a NULL argument instead of reporting it. Replace also dropped the return value of the AddString that appends the tail after the last match, while checking it inside the loop. Both are checked now. The header documented only "New size of this string object" for these methods, with nothing about error returns or empty input. The contract is now written down: an empty append or insert is a no-op returning the unchanged size, NULL returns -1, an out of bound index returns -1, and an empty alter passed to Replace removes every occurrence of the needle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CMUTIL_StringAddNStringfolded two unrelated conditions into one branch: a NULL pointer, which is a genuine programming error, and a size of zero, which is not. Every empty append returned-1and logged an ERROR with a stack trace.The consequence surfaced through
Replace. Removing a substring is spelledReplace(needle, ""), and the empty replacement madeAddStringfail, sendingReplaceto its failure path — it destroyed the result and returnedNULL. Stripping the dash out of"UTF-8"is exactly the combination that broke, and callers that dereferenced the result crashed. This killed libcmdbm's MySQL/MariaDB module in normal use.Changes
AddNStringrejectsNULLand returns the current size for a zero-size append.AddString,ReplaceandXmlSetNameall follow from that one change.InsertNStringcarried the same defect, fixed the same way — with the index validated before the size, because an out of bound index stays an error even when there is nothing to insert.InsertStringandInsertAnotherwere missing the NULL guard theirAddcounterparts already had, and crashed on a NULL argument instead of reporting it.Replacedropped the return value of theAddStringthat appends the tail after the last match, while checking it inside the loop. Both are checked now.Tests
Regression coverage in
string_test: empty and NULL append/insert, out of bound index with nothing to insert,Replace("UTF-8", "-", "")=="UTF8", needle at the start, at the end and consecutive ("--a--b--"→"ab"), and the source left unchanged.xml_testcoversSetName("").ctest15/15, no build warnings.