Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/2] fiber: prevent fiber_join from misuse
@ 2021-04-26 10:09 Cyrill Gorcunov via Tarantool-patches
  2021-04-26 10:09 ` [Tarantool-patches] [PATCH 1/2] fiber: fiber_join -- drop redundat variable Cyrill Gorcunov via Tarantool-patches
  2021-04-26 10:10 ` [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse Cyrill Gorcunov via Tarantool-patches
  0 siblings, 2 replies; 6+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-26 10:09 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

issue https://github.com/tarantool/tarantool/issues/6046
branch gorcunov/gh-6046-fiber-join

Cyrill Gorcunov (2):
  fiber: fiber_join -- drop redundat variable
  fiber: fiber_join -- don't crash on misuse

 src/lib/core/fiber.c | 8 +++++---
 test/unit/fiber.cc   | 3 +++
 2 files changed, 8 insertions(+), 3 deletions(-)


base-commit: f998ea39e96d93113823d92727a1faf9860c8ea6
-- 
2.30.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Tarantool-patches] [PATCH 1/2] fiber: fiber_join -- drop redundat variable
  2021-04-26 10:09 [Tarantool-patches] [PATCH 0/2] fiber: prevent fiber_join from misuse Cyrill Gorcunov via Tarantool-patches
@ 2021-04-26 10:09 ` Cyrill Gorcunov via Tarantool-patches
  2021-04-26 10:10 ` [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse Cyrill Gorcunov via Tarantool-patches
  1 sibling, 0 replies; 6+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-26 10:09 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

No need for additional variable here.

In-scope-of #6046

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/fiber.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index f8b85d99d..baf78a130 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -609,8 +609,7 @@ fiber_reschedule(void)
 int
 fiber_join(struct fiber *fiber)
 {
-	int rc = fiber_join_timeout(fiber, TIMEOUT_INFINITY);
-	return rc;
+	return fiber_join_timeout(fiber, TIMEOUT_INFINITY);
 }
 
 int
-- 
2.30.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse
  2021-04-26 10:09 [Tarantool-patches] [PATCH 0/2] fiber: prevent fiber_join from misuse Cyrill Gorcunov via Tarantool-patches
  2021-04-26 10:09 ` [Tarantool-patches] [PATCH 1/2] fiber: fiber_join -- drop redundat variable Cyrill Gorcunov via Tarantool-patches
@ 2021-04-26 10:10 ` Cyrill Gorcunov via Tarantool-patches
  2021-04-26 20:58   ` Vladislav Shpilevoy via Tarantool-patches
  2021-04-26 20:59   ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 2 replies; 6+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-26 10:10 UTC (permalink / raw)
  To: tml; +Cc: Vladislav Shpilevoy

In case if we call jiber_join() over the nonjoinable fiber
we trigger an assert and crash execution (on debug build).

On release build the asserts will be zapped and won't cause
problems but there is an another one -- the target fiber will
cause double fiber_reset() calls which in result cause to
unregister_fid with id = 0 (not causing crash but definitely
out of intention) and we will drop stack protection which
might be not ours anymore.

Thus lets return error just like Lua interface does. Since
nobody complained about this bug yet I think such combination
is not use commonly in external C modules and we can change
the API behaviour.

Fixes #6046

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/fiber.c | 5 ++++-
 test/unit/fiber.cc   | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index baf78a130..dd7498dd7 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -615,7 +615,10 @@ fiber_join(struct fiber *fiber)
 int
 fiber_join_timeout(struct fiber *fiber, double timeout)
 {
-	assert(fiber->flags & FIBER_IS_JOINABLE);
+	if (!(fiber->flags & FIBER_IS_JOINABLE)) {
+		diag_set(IllegalParams, "the fiber is not joinable");
+		return -1;
+	}
 
 	if (! fiber_is_dead(fiber)) {
 		bool exceeded = false;
diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
index 9c1a23bdd..fbdd82772 100644
--- a/test/unit/fiber.cc
+++ b/test/unit/fiber.cc
@@ -96,6 +96,9 @@ fiber_join_test()
 	header();
 
 	struct fiber *fiber = fiber_new_xc("join", noop_f);
+	/* gh-6046: crash on attempt to join non joinable */
+	fiber_set_joinable(fiber, false);
+	fiber_join(fiber);
 	fiber_set_joinable(fiber, true);
 	fiber_wakeup(fiber);
 	fiber_join(fiber);
-- 
2.30.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse
  2021-04-26 10:10 ` [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse Cyrill Gorcunov via Tarantool-patches
@ 2021-04-26 20:58   ` Vladislav Shpilevoy via Tarantool-patches
  2021-04-26 21:36     ` Cyrill Gorcunov via Tarantool-patches
  2021-04-26 20:59   ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 1 reply; 6+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-26 20:58 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Hi! Thanks for the patch!

> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index baf78a130..dd7498dd7 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -615,7 +615,10 @@ fiber_join(struct fiber *fiber)
>  int
>  fiber_join_timeout(struct fiber *fiber, double timeout)
>  {
> -	assert(fiber->flags & FIBER_IS_JOINABLE);
> +	if (!(fiber->flags & FIBER_IS_JOINABLE)) {

1. https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style

	In C we don't apply ! operator to non-boolean values. It means, to
	check if an integer is not 0, you use != 0. To check if a pointer is
	not NULL, you use != NULL. The same for ==;

> +		diag_set(IllegalParams, "the fiber is not joinable");
> +		return -1;
> +	}
>  
>  	if (! fiber_is_dead(fiber)) {
>  		bool exceeded = false;
> diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
> index 9c1a23bdd..fbdd82772 100644
> --- a/test/unit/fiber.cc
> +++ b/test/unit/fiber.cc
> @@ -96,6 +96,9 @@ fiber_join_test()
>  	header();
>  
>  	struct fiber *fiber = fiber_new_xc("join", noop_f);
> +	/* gh-6046: crash on attempt to join non joinable */
> +	fiber_set_joinable(fiber, false);
> +	fiber_join(fiber);

2. Would be good to test that it returns -1, and that the diag is not
empty.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse
  2021-04-26 10:10 ` [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse Cyrill Gorcunov via Tarantool-patches
  2021-04-26 20:58   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-26 20:59   ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 0 replies; 6+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-04-26 20:59 UTC (permalink / raw)
  To: Cyrill Gorcunov, tml

Also you can remove

	if (!(fiber->flags & FIBER_IS_JOINABLE))

from lua/fiber.c and check result of fiber_join() so as
not to touch fiber internals there.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse
  2021-04-26 20:58   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-04-26 21:36     ` Cyrill Gorcunov via Tarantool-patches
  0 siblings, 0 replies; 6+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-04-26 21:36 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tml

On Mon, Apr 26, 2021 at 10:58:16PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> > index baf78a130..dd7498dd7 100644
> > --- a/src/lib/core/fiber.c
> > +++ b/src/lib/core/fiber.c
> > @@ -615,7 +615,10 @@ fiber_join(struct fiber *fiber)
> >  int
> >  fiber_join_timeout(struct fiber *fiber, double timeout)
> >  {
> > -	assert(fiber->flags & FIBER_IS_JOINABLE);
> > +	if (!(fiber->flags & FIBER_IS_JOINABLE)) {
> 
> 1. https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style
> 
> 	In C we don't apply ! operator to non-boolean values. It means, to
> 	check if an integer is not 0, you use != 0. To check if a pointer is
> 	not NULL, you use != NULL. The same for ==;

I know. I left it this way on a purpose -- the whole file follows more
sane use of negation operator. I thought better to keep the old form,
otherwise we will have a mixture of old a new style in one file.

> >  	struct fiber *fiber = fiber_new_xc("join", noop_f);
> > +	/* gh-6046: crash on attempt to join non joinable */
> > +	fiber_set_joinable(fiber, false);
> > +	fiber_join(fiber);
> 
> 2. Would be good to test that it returns -1, and that the diag is not
> empty.

Will update, thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-04-26 21:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-26 10:09 [Tarantool-patches] [PATCH 0/2] fiber: prevent fiber_join from misuse Cyrill Gorcunov via Tarantool-patches
2021-04-26 10:09 ` [Tarantool-patches] [PATCH 1/2] fiber: fiber_join -- drop redundat variable Cyrill Gorcunov via Tarantool-patches
2021-04-26 10:10 ` [Tarantool-patches] [PATCH 2/2] fiber: fiber_join -- don't crash on misuse Cyrill Gorcunov via Tarantool-patches
2021-04-26 20:58   ` Vladislav Shpilevoy via Tarantool-patches
2021-04-26 21:36     ` Cyrill Gorcunov via Tarantool-patches
2021-04-26 20:59   ` Vladislav Shpilevoy via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git