[Patch] Setup: Warn about dropped mirrors.

[Christopher Faylor]

On Tue, Mar 14, 2006 at 01:40:00AM -0000, Dave Korn wrote:
>On 10 March 2006 17:20, Christopher Faylor wrote:
>
>> On Mon, Mar 06, 2006 at 05:00:45AM +0100, Bas van Gompel wrote:
>>> Op Wed, 16 Nov 2005 23:29:50 +0100 (MET) schreef Bas van Gompel
>>> in <n2m-g.dlgdit.3vsvtmt.1<at>buzzy-box.bavag>:
>>> [Warn about dropped mirrors]
>>> 
>>> One more iteration. This is what I've been testing/using for months
>>> now. Before I start changing/optimizing this, I'd like to get it in.
>>> 
>>> [Oh and a big PING on those other setup-patches from me (and Igor[*]).]
>>> 
>>> Now using std::string, not String. 
>
>
>  Is that ok vs the passing std::string params to dll functions in 3.4.4
>issue?  
>
>
>>   (gdb) f 2
>>   #2  0x0045293c in do_download_site_info_thread (p=0x4dbd40) at
>>   /cygnus/src/cygwin-apps/setup/site.cc:330 330         theCachedString =
>> new_cstr_char_array (cached_mirrors); 
>> 
>> This happened a couple of times and then I could no longer duplicate it.
>> 
>> Can anyone else?
>
>  I can reproduce this by renaming aside mirrors-lst.  It occurs on the line
>
>theCachedString = new_cstr_char_array (cached_mirrors);
>
>when cached_mirrors is an empty string.  
>
><ASIDE>
>  <sigh> This is why I hate C++ so much:
>
>	string cached_mirrors = "";
>
>  That's a std::string.  It's basically a bit of dynamically allocated memory
>and a count of the current size.  Maybe a ref count to, but basically it's a
>counted buffer.
>
>	char *theMirrorString, *theCachedString;
>	theCachedString = new_cstr_char_array (cached_mirrors);
>
>  That's an attempt to get an ordinary C-style string out of it.
>Unfortunately...
>
>	char *
>	new_cstr_char_array (const String &s)
>
>... that's a function that takes a different type and requires a conversion
>constructor.
>
>  So, in order to get a copy of the contents of the buffer in the std::string,
>we're dynamically declaring a temporary String, which is allocating a new
>buffer, then copying it into the String's buffer from the std::string's
>buffer, then calling a routine which dynamically allocates the same amount of
>memory again and copies the same contents again from the String's buffer to
>the new buffer and returns it.  Then the String goes out of context and
>deallocates its buffer.  Then the std::string is manually deleted and
>deallocates its buffer.
>
>  Say, did I mention I hate c++ ?  :-/
></ASIDE>
>
>  Anyway what happens is that a temp String object is constructed with no
>contents, and when we get to new_cstr_char_array, we run into a bug:
>
>char *
>new_cstr_char_array (const String &s)
>{
>  size_t len = s.size() + 1;
>  char *buf = new char[len];
>  memcpy (buf, s.c_str (), len);
>  return buf;
>}
>
>  Because String objects store strings with an explicit length and don't count
>the nul-terminator, this routine adds one to the s.size() to allow for the
>nul-terminator in the final char* array.  It then copies that size of data
>from the String object in order to get the uncounted nul-terminator.
>
>  Unfortunately, String objects not just don't count the nul-term, they don't
>even store it.  And if they have a length of zero, then c_str returns NULL,
>not a pointer to a zero-length buffer, nor (as the above code seems to expect)
>a pointer to a one-byte buffer containing a NUL.  So s.c_str is NULL and hence
>the SEGV in the memcpy.
>
>  One possible fix is the first attached patch.  Would check it in as trivial
>and obvious, but I don't know whether we should rightly consider this a bug in
>c_str rather than in new_cstr_char_array; it's not like we have a
>tightly-documented spec for the String class that would let us decide whether
>n_c_c_a is at fault for not handling a valid return or c_str is at fault for
>returning an invalid value.  Maybe we want to think about whether we should
>make String store the nul terminator after all, or otherwise not let
>String::c_str() return NULL?  It could just as easily return "" instead.  I've
>attached three possible patches; the first one is the one that most directly
>attacks the problem at site with least danger of knock-on effects, whereas if
>we change c_str we'd have to be sure that any code that depended on getting a
>NULL return worked as well when given a real return with a strlen of zero.
>
>
>2006-03-14  Dave Korn  <dave.korn@artimi.com>
>
>	* String++.cc (new_cstr_char_array):  Handle null input correctly.
>
>2006-03-14  Dave Korn  <dave.korn@artimi.com>
>
>	* String++.cc (String::c_str):  Return a pointer to a null string
>  constant rather than NULL when String's length is zero.
>
>2006-03-14  Dave Korn  <dave.korn@artimi.com>
>
>	* String++.cc (String::c_str):  Skip optimisation for zero-length
>  strings and still allocate a 1-byte cstr buffer with a nul-terminator.

Unless there are objections within the next 24 hours, I'd say "go ahead".

Thanks for tracking this down.  I thought it was a problem like this but
I also hate the excessive use of c++ like this and was hoping against magical
hope that someone else would fix this.

cgf