This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [Patch] Setup: Warn about dropped mirrors.


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.

    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....

Attachment: c_str-bugfix-patch.diff
Description: Binary data

Attachment: c_str-bugfix3-patch.diff
Description: Binary data

Attachment: c_str-bugfix2-patch.diff
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]