Page MenuHomeFreeBSD
Differential D18939

Add pthread to lang/lua53
ClosedPublic
Actions

Authored by russ.haley_gmail.com on Jan 24 2019, 5:53 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 7, 2:56 AM
Unknown Object (File)
Sun, Apr 7, 1:56 AM
Unknown Object (File)
Fri, Mar 29, 1:33 AM
Unknown Object (File)
Mon, Mar 25, 4:32 AM
Unknown Object (File)
Mar 18 2024, 4:54 PM
Unknown Object (File)
Feb 5 2024, 2:42 PM
Unknown Object (File)
Jan 15 2024, 2:15 PM
Unknown Object (File)
Dec 23 2023, 4:17 PM
View All 69 Files
Subscribers
i_fbsd_amlegion.org
mat

Details

Reviewers
andrew_tao173.riddles.org.uk
dbn
Commits
rP496471: lang/lua: workaround dlopen/libthr bug on FreeBSD 12
Summary

There was a regression discoverd on 12-RELEASE: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235211

The bug was first identified with Lua53 with cqueues: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235158

This patch adds pthread to LIBS until it's prudent to take it out. This patch does not affect the building of shared object files.

Test Plan

The test code succeeds in FreeBSD 11.1 and fails on 12.

Note: This test requires cqueues to be available to Lua53.

The following cqueues code causes Lua 53 to hang on 12-Release. The word data is printed when successful on 11.1.

  local cqueues = require 'cqueues'
  local thread = require 'cqueues.thread'
  
  local function print_data(sock, data) print(data) end
  local function start() 
		local thr,sock = thread.start(print_data, 'data') 
		thr:join()
        end
  local function test() print('test') end
  local loop = cqueues.new()

  loop:wrap(start)
  --loop:wrap(test)
  loop:loop()

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

russ.haley_gmail.com created this revision.Jan 24 2019, 5:53 AM
Herald added a subscriber: mat. · View Herald TranscriptJan 24 2019, 5:53 AM
linimon retitled this revision from Add pthread Option to Lua53 Port to Add pthread Option to lang/lua53.Jan 24 2019, 5:58 AM
russ.haley_gmail.com edited the test plan for this revision. (Show Details)Jan 24 2019, 6:06 AM
russ.haley_gmail.com updated this revision to Diff 53141.Jan 24 2019, 6:09 AM

Removed commented out code.

andrew_tao173.riddles.org.uk added a comment.Jan 24 2019, 7:43 AM

Russ, did you check whether that test actually failed without the patch? Because it works fine for me without changing a thing.

mat added a comment.Jan 24 2019, 8:11 AM

Could you:

  1. not change other things that what you are adding (remove the whitespace changes)
  2. use devel/arcanist, or at least generate a diff with full context like it does, with svn diff -x -U9999 or git diff -U9999.
i_fbsd_amlegion.org added a subscriber: i_fbsd_amlegion.org.Jan 24 2019, 1:08 PM
russ.haley_gmail.com updated this revision to Diff 53182.Jan 25 2019, 5:15 AM

Removed whitespace changes, used recommended svn diff options.

andrew_tao173.riddles.org.uk added a comment.Jan 25 2019, 4:39 PM

I think in the short term there's no point in adding an option, and instead we should just append -lpthread to LIBS, since we ignore LIBS when building the .so anyway.

(I've tried this and the dependencies all seem to come out right.)

Having the lua53 binary pull in libthr is certainly harmless (it has no effect unless someone loads a module that uses threads, in which case they're referencing it anyway) and if it's a sufficient workaround for the bug, then why not.

russ.haley_gmail.com added a comment.Jan 25 2019, 5:01 PM
This comment was removed by russ.haley_gmail.com.
russ.haley_gmail.com added a comment.Jan 25 2019, 5:02 PM
In D18939#405045, @andrew_tao173.riddles.org.uk wrote:

I think in the short term there's no point in adding an option, and instead we should just append -lpthread to LIBS, since we ignore LIBS when building the .so anyway.

(I've tried this and the dependencies all seem to come out right.)

Having the lua53 binary pull in libthr is certainly harmless (it has no effect unless someone loads a module that uses threads, in which case they're referencing it anyway) and if it's a sufficient workaround for the bug, then why not.

Sounds good. I'll clean that up tonight.

russ.haley_gmail.com updated this revision to Diff 53238.EditedJan 26 2019, 5:24 AM

Removed -pthread options. Added -pthread to LIBS.

andrew_tao173.riddles.org.uk added a comment.Jan 26 2019, 5:37 AM

While we're in here changing stuff, is it also worth adding a patch for the upvaluejoin bug? The bug in itself is a non-issue, but some prat filed a CVE for it.

andrew_tao173.riddles.org.uk added inline comments.Jan 26 2019, 6:00 AM
Makefile
105

Perhaps a comment that this is done only as a workaround for bug 253211, and that pthreads are not otherwise needed?

russ.haley_gmail.com updated this revision to Diff 53267.Jan 27 2019, 5:50 AM

Added comment noting bug.

russ.haley_gmail.com edited the test plan for this revision. (Show Details)Jan 27 2019, 5:53 AM
russ.haley_gmail.com edited the test plan for this revision. (Show Details)
andrew_tao173.riddles.org.uk added inline comments.Jan 27 2019, 10:31 AM
Makefile
5

As I understand it we need PORTREVISION=1 here now immediately after DISTVERSION, since this is a change to the port.

russ.haley_gmail.com updated this revision to Diff 53292.Jan 28 2019, 5:50 AM

Add PORTVERSION.

russ.haley_gmail.com marked an inline comment as done.Jan 29 2019, 5:50 AM
russ.haley_gmail.com marked an inline comment as done.
russ.haley_gmail.com retitled this revision from Add pthread Option to lang/lua53 to Add pthread to lang/lua53.Jan 29 2019, 5:56 AM
russ.haley_gmail.com edited the summary of this revision. (Show Details)
russ.haley_gmail.com edited the test plan for this revision. (Show Details)
russ.haley_gmail.com added a comment.Feb 16 2019, 4:54 AM
In D18939#405379, @andrew_tao173.riddles.org.uk wrote:

While we're in here changing stuff, is it also worth adding a patch for the upvaluejoin bug? The bug in itself is a non-issue, but some prat filed a CVE for it.

Hi, sorry I missed this comment earlier.

Reference: https://nvd.nist.gov/vuln/detail/CVE-2019-6706

Roberto doesn't seem fond of the patch put forward here: http://lua-users.org/lists/lua-l/2019-01/msg00110.html

There are no patches for the CVE on the Lua.org site (https://www.lua.org/bugs.html) so my preference would be to get this pushed into svn and patch for the CVE when Lua releases something. Thoughts?

I've tested on 11.2 and 12. The patch conclusively fixes the issue on 12-RELEASE.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 21 2019, 4:26 PM
Closed by commit rP496471: lang/lua: workaround dlopen/libthr bug on FreeBSD 12 (authored by dbn). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
1 line

Diff 53238

View Options

Makefile

Loading...