Re: [PATCH] pthread_fork Part 2

[Robert Collins <rbcollins at cygwin dot com>]

On Sat, 2002-08-17 at 06:32, Thomas Pfaff wrote:
> 
> Some small fixes in the pthread key handling.
> @@ -1020,16 +1020,27 @@ pthread_key::~pthread_key ()
>  int
>  pthread_key::set (const void *value)
>  {
> -  /*the OS function doesn't perform error checking */
> -  TlsSetValue (dwTlsIndex, (void *) value);
> +  if (dwTlsIndex == TLS_OUT_OF_INDEXES ||

Not needed. dwTlsIndex is not set by anyone outside the class, AND
if TlsAlloc fails, then we set the magic to 0, causing a failure on
creation. 

Are you covering the situation where the restoreFromSavedBuffer call
fails? If so, then we should cause the object to destroy itself in that
call, thus causing the VALID_OBJECT test to fail for future calls from
userland.

> +      !TlsSetValue (dwTlsIndex, (void *) value))

Please see the MS documentation on this. They explicitly state that they
perform minimal checking. Also, this should be an assert, as TlsSetValue
can only fail if you give it an invalid index, and our index is assigned
by the OS.

> +  if (dwTlsIndex == TLS_OUT_OF_INDEXES)

Ditto to above.

> +    result = TlsGetValue (dwTlsIndex);

And again.

>  
>  void
> @@ -1884,8 +1895,8 @@ __pthread_setspecific (pthread_key_t key
>  {
>    if (verifyable_object_isvalid (&key, PTHREAD_KEY_MAGIC) != VALID_OBJECT)
>      return EINVAL;
> -  (key)->set (value);
> -  return 0;
> +
> +  return (key)->set (value);

Not needed, because of the above lack of changes.
Yes, what you are suggesting is good general programming practice, but
these are performance critical functions, and we are checking for a
situation that can't happen short of someone writing into our memory
space. If that happens, errors are the least of our problems :}.

Rob

Attachment: signature.asc
Description: This is a digitally signed message part