On 15/04/2014 2:56 a.m., Alex Rousskov wrote:
> On 04/14/2014 04:49 AM, Amos Jeffries wrote:
>> I've added a unit test to catch the rare \0 mid-string case I spoke of:
>> * SBuf("foo\0test").compare("foo", 9);
>>
>> It works fine up to the point N(4) > strlen("foo"). After that point our
>> function returns 1 to indicate that the SBuf is a longer string, whereas
>> strncmp/strncasecmp return 0 up to infinity.
>
> Yes, this is related to the large-n handling bug I keep talking about.
> IMO, this must be fixed as previously discussed: C-string API should not
> look past the first null character.
>
>
>> The code as presented earlier copes with the weirdness fine.
>
>
> AFAICT, the latest posted patch accesses non-existent c-string bytes
> under certain conditions (large n, large SBuf with trailing null
> characters, short s matching the c-string in SBuf). Do you agree? If you
> do agree, please post a fixed patch. If not, I would have to spend time
> writing a test case to prove my point against the last patch posted.
I agree.
Attached patch implements what we agreed on in IRC.
It produces wrong return value in two case states. Marked with "BUG 1"
and "BUG 2" in the patch.
Amos
=== modified file 'src/SBuf.cc'
--- src/SBuf.cc 2014-04-06 07:08:04 +0000
+++ src/SBuf.cc 2014-04-14 16:53:12 +0000
@@ -360,64 +360,98 @@
store_->mem[off_+pos] = toset;
++stats.setChar;
}
static int
memcasecmp(const char *b1, const char *b2, SBuf::size_type len)
{
int rv=0;
while (len > 0) {
rv = tolower(*b1)-tolower(*b2);
if (rv != 0)
return rv;
++b1;
++b2;
--len;
}
return rv;
}
int
-SBuf::compare(const SBuf &S, SBufCaseSensitive isCaseSensitive, size_type n)
const
+SBuf::compare(const SBuf &S, const SBufCaseSensitive isCaseSensitive, const
size_type n) const
{
if (n != npos)
return substr(0,n).compare(S.substr(0,n),isCaseSensitive);
- size_type byteCompareLen = min(S.length(), length());
+ const size_type byteCompareLen = min(S.length(), length());
++stats.compareSlow;
int rv = 0;
if (isCaseSensitive == caseSensitive) {
rv = memcmp(buf(), S.buf(), byteCompareLen);
} else {
rv = memcasecmp(buf(), S.buf(), byteCompareLen);
}
if (rv != 0)
return rv;
if (length() == S.length())
return 0;
if (length() > S.length())
return 1;
return -1;
}
+int
+SBuf::compare(const char *s, const SBufCaseSensitive isCaseSensitive, const
size_type n) const
+{
+ // 0-length comparison is always true regardless of buffer states
+ if (!n) {
+ ++stats.compareFast;
+ return 0;
+ }
+
+ // N-length compare MUST provide a non-NULL C-string pointer
+ assert(s);
+
+ // recurse after finding length if unknown (including terminator byte)
+ if (n == npos)
+ return compare(s, isCaseSensitive, strlen(s)+1);
+
+ // if this SBuf is bigger than N truncate it.
+ // guaranteeing length() <= n for the following comparison
+ if (length() > n)
+ return substr(0,n).compare(s, isCaseSensitive, n);
+
+ const size_type byteCompareLen = min(n, length());
+ ++stats.compareSlow;
+ int rv = 0;
+ if (isCaseSensitive == caseSensitive) {
+ rv = strncmp(buf(), s, byteCompareLen);
+ } else {
+ rv = strncasecmp(buf(), s, byteCompareLen);
+ }
+ // BUG 1: when length() < n - buffer overruns on buf().
+ // BUG 2: when length() == strlen(s) < n, no terminator to match against
in buf()
+ return rv;
+}
+
bool
-SBuf::startsWith(const SBuf &S, SBufCaseSensitive isCaseSensitive) const
+SBuf::startsWith(const SBuf &S, const SBufCaseSensitive isCaseSensitive) const
{
debugs(24, 8, id << " startsWith " << S.id << ", caseSensitive: " <<
isCaseSensitive);
if (length() < S.length()) {
debugs(24, 8, "no, too short");
++stats.compareFast;
return false;
}
return (compare(S, isCaseSensitive, S.length()) == 0);
}
bool
SBuf::operator ==(const SBuf & S) const
{
debugs(24, 8, id << " == " << S.id);
if (length() != S.length()) {
debugs(24, 8, "no, different lengths");
++stats.compareFast;
return false; //shortcut: must be equal length
}
=== modified file 'src/SBuf.h'
--- src/SBuf.h 2014-04-06 07:08:04 +0000
+++ src/SBuf.h 2014-04-13 04:05:25 +0000
@@ -238,58 +238,71 @@
char at(size_type pos) const {checkAccessBounds(pos); return
operator[](pos);}
/** direct-access set a byte at a specified operation.
*
* \param pos the position to be overwritten
* \param toset the value to be written
* \throw OutOfBoundsException when pos is of bounds
* \note bounds is 0 <= pos < length(); caller must pay attention to
signedness
* \note performs a copy-on-write if needed.
*/
void setAt(size_type pos, char toset);
/** compare to other SBuf, str(case)cmp-style
*
* \param isCaseSensitive one of caseSensitive or caseInsensitive
* \param n compare up to this many bytes. if npos (default), compare
whole SBufs
* \retval >0 argument of the call is greater than called SBuf
* \retval <0 argument of the call is smaller than called SBuf
* \retval 0 argument of the call has the same contents of called SBuf
*/
- int compare(const SBuf &S, SBufCaseSensitive isCaseSensitive, size_type n
= npos) const;
+ int compare(const SBuf &S, const SBufCaseSensitive isCaseSensitive, const
size_type n = npos) const;
- /// shorthand version for compare
- inline int cmp(const SBuf &S, size_type n = npos) const {
+ /// shorthand version for compare()
+ inline int cmp(const SBuf &S, const size_type n = npos) const {
return compare(S,caseSensitive,n);
}
- /// shorthand version for case-insensitive comparison
- inline int caseCmp(const SBuf &S, size_type n = npos) const {
+ /// shorthand version for case-insensitive compare()
+ inline int caseCmp(const SBuf &S, const size_type n = npos) const {
+ return compare(S,caseInsensitive,n);
+ }
+
+ /// comparison with a C-string
+ int compare(const char *s, const SBufCaseSensitive isCaseSensitive, const
size_type n) const;
+
+ /// shorthand version for C-string compare()
+ inline int cmp(const char *S, const size_type n = npos) const {
+ return compare(S,caseSensitive,n);
+ }
+
+ /// shorthand version for case-insensitive C-string compare()
+ inline int caseCmp(const char *S, const size_type n = npos) const {
return compare(S,caseInsensitive,n);
}
/** check whether the entire supplied argument is a prefix of the SBuf.
* \param S the prefix to match against
* \param isCaseSensitive one of caseSensitive or caseInsensitive
* \retval true argument is a prefix of the SBuf
*/
- bool startsWith(const SBuf &S, SBufCaseSensitive isCaseSensitive =
caseSensitive) const;
+ bool startsWith(const SBuf &S, const SBufCaseSensitive isCaseSensitive =
caseSensitive) const;
bool operator ==(const SBuf & S) const;
bool operator !=(const SBuf & S) const;
bool operator <(const SBuf &S) const {return (cmp(S) < 0);}
bool operator >(const SBuf &S) const {return (cmp(S) > 0);}
bool operator <=(const SBuf &S) const {return (cmp(S) <= 0);}
bool operator >=(const SBuf &S) const {return (cmp(S) >= 0);}
/** Consume bytes at the head of the SBuf
*
* Consume N chars at SBuf head, or to SBuf's end,
* whichever is shorter. If more bytes are consumed than available,
* the SBuf is emptied
* \param n how many bytes to remove; could be zero.
* npos (or no argument) means 'to the end of SBuf'
* \return a new SBuf containing the consumed bytes.
*/
SBuf consume(size_type n = npos);
/// gets global statistic informations
