Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix segmentation fault when using jq in threads #2546

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

thalman
Copy link
Contributor

@thalman thalman commented Mar 1, 2023

In previous code nomem_handler in jv_alloc.c is called only once and therefore the structure is not initialized for second and following threads.

This leads to segmentation fault in multi-threading environment.

This patch moves initialization of nomem_handler out of the pthread_once call.

This patch fixes the issue #2545

@edwardb96
Copy link

edwardb96 commented Mar 3, 2023

I think this is related to #2483 and #2484 - but given it's been 6 months I'm not sure I'd hold out much hope.

@thalman
Copy link
Contributor Author

thalman commented Mar 6, 2023

@edwardb96 I see, you are right. This is the same issue. I do not mind if my patch is not selected, but what prevents merging one of those?

I'm about to include one of them to Fedora and RHEL anyway.

Tom

@wader
Copy link
Member

wader commented Mar 6, 2023

Hi, thanks both for the PRs, see #2305 for current state of the project

@nicowilliams
Copy link
Contributor

Let's make sure we review this and related issues/PRs before we release, no? I'll see if I can find time this week.

@wader
Copy link
Member

wader commented Jul 5, 2023

Maybe this one should be added to https://github.com/jqlang/jq/milestones/1.7%20release? it's the list i've picked things from.

@itchyny wrote about a release plan here #2623 (comment)

@itchyny
Copy link
Contributor

itchyny commented Jul 5, 2023

@nicowilliams Thank you. Very appreciated, and I added this to the milestone.

@itchyny itchyny added this to the 1.7 release milestone Jul 5, 2023
@thalman
Copy link
Contributor Author

thalman commented Jul 7, 2023

Thank you, guys for taking care of this issue. Let me know if you need anything from my side.

@wader
Copy link
Member

wader commented Jul 24, 2023

@thalman Hi, can you rebase on master? the CI should work better now

In previous code nomem_handler in jv_alloc.c is called only once
and therefore the structure is not initialized for second and
following threads.

This leads to segmentation fault in multi-threading environment.

This patch moves initialization of nomem_handler out of the
pthread_once call.
This patch implements test that searches a key in simple
json in pthread.
@thalman
Copy link
Contributor Author

thalman commented Jul 24, 2023

@thalman Hi, can you rebase on master? the CI should work better now

@wader, rebased. Thanks for the CI work!

@wader
Copy link
Member

wader commented Jul 24, 2023

Do you know if this is a regression? i haven't linked with libjq myself but looking at the jq code it seems to have quite a lot of code to be thread safe, but was is never really tested or it broke at some point?

Thanks for adding a test btw!

@thalman
Copy link
Contributor Author

thalman commented Jul 24, 2023

The bug is not present in 1.5. It was introduced later on in commit 3f86e97

I would not call that regression. It brings a new functionality that allows different no_mem_handler for different threads, it just did not work properly and it was not tested in more threads.

@nicowilliams nicowilliams merged commit cca0087 into jqlang:master Jul 24, 2023
27 checks passed
@nicowilliams
Copy link
Contributor

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants