[Patch] Fix return value in pthread_create

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch] Fix return value in pthread_create

Jeff Law
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


pthread_create is supposed to return EAGAIN if there are insufficient
resources to create the new thread.

This problem was partially fixed in 0f7e0ee5, BZ 5245.  However it
missed the case where there's a failure allocating the thread's stack.

2011-12-14  Jeff Law  <[hidden email]>

        * pthread_create.c (pthread_create_2_1): Translate ENOMEM to
        EAGAIN.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJO6Qw4AAoJEBRtltQi2kC7+DwH/i/xadL/BYexBpPolZ8i2LCP
v+GPMP4lc2FN5sZTvERRDU9eDiUbmDrWH94jw/NESXoJLCQK0rA0DbWOYLopLTaJ
Kwai3K4QFX+SJwYL7it+o/lt7vKl6a89kMcr3iR8WM3MNsmIrzMxxLxI9/EJcrbn
6pc3kDZL7fm1F7RBDnOBC31tF0ptObyL21GMPmMOpfYIXSa+9E32yC/TQYfQtNA4
hFkPvguq6KuFnIeC2HYvuecI11vbbR623YUz6zLoUV1eTMibxnZg2eH+A60UkYWQ
+/6ahki1/CXI8T76jCWe0veReS2cDChPQ0dINLGfcmPorZcv7AqFHctKZKgedfQ=
=pM4/
-----END PGP SIGNATURE-----

glibc-threadcreateretval.patch (597 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix return value in pthread_create

Roland McGrath-4
Applied, with log entry fix (wrong function name and BZ# tag).

Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix return value in pthread_create

Carlos O'Donell-2
On Wed, Dec 14, 2011 at 4:16 PM, Roland McGrath <[hidden email]> wrote:
> Applied, with log entry fix (wrong function name and BZ# tag).

If we are going to check for ENOMEM in pthread_create.c then
allocatestack.c should be fixed to stop checking for the same
condition.

I'm reg-testing the attached patch on i686-pc-linux-gnu.

Is there any reason not to apply the patch if the reg-testing turns up OK?

Cheers,
Carlos.

allocatestack.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix return value in pthread_create

Roland McGrath-4
> If we are going to check for ENOMEM in pthread_create.c then
> allocatestack.c should be fixed to stop checking for the same
> condition.

I gave Jeff the benefit of the doubt and was too lazy to hunt down that
code.  If it's checked adequately there then there's no reason for the
change I made.  But since it's already in, and it has the translation in
one place instead of five, that plus your change seems ultimately better.
Go ahead and commit it if you don't find any problems.

The only potential caveat is that there is one path where we were changing
errno as well.  But since the other four paths don't do that, I guess it
doesn't matter what's in errno.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix return value in pthread_create

Carlos O'Donell-2
On Wed, Dec 14, 2011 at 5:00 PM, Roland McGrath <[hidden email]> wrote:

>> If we are going to check for ENOMEM in pthread_create.c then
>> allocatestack.c should be fixed to stop checking for the same
>> condition.
>
> I gave Jeff the benefit of the doubt and was too lazy to hunt down that
> code.  If it's checked adequately there then there's no reason for the
> change I made.  But since it's already in, and it has the translation in
> one place instead of five, that plus your change seems ultimately better.
> Go ahead and commit it if you don't find any problems.
>
> The only potential caveat is that there is one path where we were changing
> errno as well.  But since the other four paths don't do that, I guess it
> doesn't matter what's in errno.

Any sensible user code will continue to work e.g. if (errno).

In fact strerror(errno) is now accurately the true errno of the last
failed library call that set errno.

The only code that might now fail is:

pthread_create(...)
if (errno == EAGAIN)
...

Which would already fail in the 4 other return paths, not to mention
that errno is undefined after a function call that doesn't declare
that it sets errno.

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix return value in pthread_create

Carlos O'Donell-2
In reply to this post by Roland McGrath-4
On Wed, Dec 14, 2011 at 5:00 PM, Roland McGrath <[hidden email]> wrote:

>> If we are going to check for ENOMEM in pthread_create.c then
>> allocatestack.c should be fixed to stop checking for the same
>> condition.
>
> I gave Jeff the benefit of the doubt and was too lazy to hunt down that
> code.  If it's checked adequately there then there's no reason for the
> change I made.  But since it's already in, and it has the translation in
> one place instead of five, that plus your change seems ultimately better.
> Go ahead and commit it if you don't find any problems.
>
> The only potential caveat is that there is one path where we were changing
> errno as well.  But since the other four paths don't do that, I guess it
> doesn't matter what's in errno.

Reg-test was clean, cleanup committed to master.

Should we also not update NEWS to reflect the new bug fix?

It looks like you've been updating NEWS just before a release, but
there's no reason to delay is there?

Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix return value in pthread_create

Roland McGrath-4
> Should we also not update NEWS to reflect the new bug fix?
>
> It looks like you've been updating NEWS just before a release, but
> there's no reason to delay is there?

Since I reused an old BZ# that was already marked fixed, there might not be
anything to add.

As a general thing, there certainly is no reason to delay adding to the
NEWS list of fixed BZ#s.  But it's reasonable and expected that people
won't do this every single time.  So it's always going to be appropriate
that at release time someone greps the ChangeLog since the last release for
BZ# tags and updates NEWS to be sure we got them all.


Thanks,
Roland

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix return value in pthread_create

Carlos O'Donell-2
On Thu, Dec 15, 2011 at 5:15 PM, Roland McGrath <[hidden email]> wrote:

>> Should we also not update NEWS to reflect the new bug fix?
>>
>> It looks like you've been updating NEWS just before a release, but
>> there's no reason to delay is there?
>
> Since I reused an old BZ# that was already marked fixed, there might not be
> anything to add.
>
> As a general thing, there certainly is no reason to delay adding to the
> NEWS list of fixed BZ#s.  But it's reasonable and expected that people
> won't do this every single time.  So it's always going to be appropriate
> that at release time someone greps the ChangeLog since the last release for
> BZ# tags and updates NEWS to be sure we got them all.

Right, that's fine, belt and suspenders. Sounds good.

Cheers,
Carlos.