[squid-dev] [PATCH] SBuf Locker friend class

[Amos Jeffries]

Adds the Locker friend class to SBuf to provide safety ref-count locks
when dealing with dangerous char* or SBuf& inputs.

I have gone through and added it to all the non-const methods that
appear to be taking char* directly, or using a char* taken from SBuf&.
That review added the appendf/printf methods to the set we identified on
IRC.

IMO the trim() method mentioned on IRC does not need it since the scope
lifetime of the pointer taken by toRemove.buf() does not cross any
alterations of the this objects members.


I've not added documentation for the Locker class. Alex could you
provide a blurb to describe the memory issue and requirements this solves ?

Amos
-------------- next part --------------
=== modified file 'src/SBuf.cc'
--- src/SBuf.cc	2015-08-12 22:18:22 +0000
+++ src/SBuf.cc	2015-10-29 18:41:39 +0000
@@ -144,40 +144,41 @@
     return InitialStore;
 }
 
 SBuf&
 SBuf::assign(const SBuf &S)
 {
     debugs(24, 7, "assigning " << id << " from " <<  S.id);
     if (&S == this) //assignment to self. Noop.
         return *this;
     ++stats.assignFast;
     store_ = S.store_;
     off_ = S.off_;
     len_ = S.len_;
     return *this;
 }
 
 SBuf&
 SBuf::assign(const char *S, size_type n)
 {
     debugs(24, 6, id << " from c-string, n=" << n << ")");
+    Locker prevent_raw_memory_madness(this, S, n);
     clear();
     return append(S, n); //bounds checked in append()
 }
 
 void
 SBuf::reserveCapacity(size_type minCapacity)
 {
     Must(minCapacity <= maxSize);
     cow(minCapacity);
 }
 
 char *
 SBuf::rawSpace(size_type minSpace)
 {
     Must(length() <= maxSize - minSpace);
     debugs(24, 7, "reserving " << minSpace << " for " << id);
     ++stats.rawAccess;
     // we're not concerned about RefCounts here,
     // the store knows the last-used portion. If
     // it's available, we're effectively claiming ownership
@@ -196,90 +197,99 @@
 void
 SBuf::clear()
 {
 #if 0
     //enabling this code path, the store will be freed and reinitialized
     store_ = GetStorePrototype(); //uncomment to actually free storage upon clear()
 #else
     //enabling this code path, we try to release the store without deallocating it.
     // will be lazily reallocated if needed.
     if (store_->LockCount() == 1)
         store_->clear();
 #endif
     len_ = 0;
     off_ = 0;
     ++stats.clear;
 }
 
 SBuf&
 SBuf::append(const SBuf &S)
 {
+    Locker prevent_raw_memory_madness(this, S.buf(), S.length());
     return lowAppend(S.buf(), S.length());
 }
 
 SBuf &
 SBuf::append(const char * S, size_type Ssize)
 {
     if (S == NULL)
         return *this;
     if (Ssize == SBuf::npos)
         Ssize = strlen(S);
     debugs(24, 7, "from c-string to id " << id);
+    Locker prevent_raw_memory_madness(this, S, Ssize);
     // coverity[access_dbuff_in_call]
     return lowAppend(S, Ssize);
 }
 
 SBuf &
 SBuf::append(const char c)
 {
     return lowAppend(&c, 1);
 }
 
 SBuf&
 SBuf::Printf(const char *fmt, ...)
 {
+    // with printf() an arg might be a dangerous char*
+    // NP: cant rely on vappendf() Locker because of clear()
+    Locker prevent_raw_memory_madness(this, buf(), length());
+
     va_list args;
     va_start(args, fmt);
     clear();
     vappendf(fmt, args);
     va_end(args);
     return *this;
 }
 
 SBuf&
 SBuf::appendf(const char *fmt, ...)
 {
     va_list args;
     va_start(args, fmt);
     vappendf(fmt, args);
     va_end(args);
     return *this;
 }
 
 SBuf&
 SBuf::vappendf(const char *fmt, va_list vargs)
 {
     Must(fmt != NULL);
     int sz = 0;
     //reserve twice the format-string size, it's a likely heuristic
     size_type requiredSpaceEstimate = strlen(fmt)*2;
 
+    // with appendf() an arg might be a dangerous char*
+    Locker prevent_raw_memory_madness(this, buf(), length());
+
     char *space = rawSpace(requiredSpaceEstimate);
 #ifdef VA_COPY
     va_list ap;
     VA_COPY(ap, vargs);
     sz = vsnprintf(space, spaceSize(), fmt, ap);
     va_end(ap);
 #else
     sz = vsnprintf(space, spaceSize(), fmt, vargs);
 #endif
 
     /* check for possible overflow */
     /* snprintf on Linux returns -1 on output errors, or the size
      * that would have been written if enough space had been available */
     /* vsnprintf is standard in C99 */
 
     if (sz >= static_cast<int>(spaceSize())) {
         // not enough space on the first go, we now know how much we need
         requiredSpaceEstimate = sz*2; // TODO: tune heuristics
         space = rawSpace(requiredSpaceEstimate);
         sz = vsnprintf(space, spaceSize(), fmt, vargs);

=== modified file 'src/SBuf.h'
--- src/SBuf.h	2015-08-12 22:18:22 +0000
+++ src/SBuf.h	2015-10-29 18:53:31 +0000
@@ -650,40 +650,54 @@
     iterator begin() {
         return iterator(*this, 0);
     }
 
     iterator end() {
         return iterator(*this, length());
     }
 
     reverse_iterator rbegin() {
         return reverse_iterator(*this, length());
     }
 
     reverse_iterator rend() {
         return reverse_iterator(*this, 0);
     }
 
     // TODO: possibly implement erase() similar to std::string's erase
     // TODO: possibly implement a replace() call
 private:
 
+    class Locker
+    {
+    public:
+        Locker(SBuf *parent, const char *Q, size_t len) : locket(nullptr) {
+            // lock if Q intersects the parents buffer area
+            const MemBlob *P = parent->store_.getRaw();
+            if ( (Q+len) >= P->mem && Q <= (P->mem + P->capacity) )
+                locket = P;
+        }
+    private:
+        MemBlob::Pointer locket;
+    };
+    friend class Locker;
+
     MemBlob::Pointer store_; ///< memory block, possibly shared with other SBufs
     size_type off_; ///< our content start offset from the beginning of shared store_
     size_type len_; ///< number of our content bytes in shared store_
     static SBufStats stats; ///< class-wide statistics
 
     /// SBuf object identifier; does not change when contents do,
     ///   including during assignment
     const InstanceId<SBuf> id;
 
     /** obtain prototype store
      *
      * Just-created SBufs all share to the same MemBlob.
      * This call instantiates and returns it.
      */
     static MemBlob::Pointer GetStorePrototype();
 
     /**
      * obtains a char* to the beginning of this SBuf in memory.
      * \note the obtained string is NOT null-terminated.
      */