Page MenuHomeFreeBSD

Add pthread to lang/lua53
ClosedPublic

Authored by russ.haley_gmail.com on Jan 24 2019, 5:53 AM.

Details

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

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

Removed commented out code.

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.

Removed whitespace changes, used recommended svn diff options.

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.

This comment was removed by russ.haley_gmail.com.

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.

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.

Makefile
105 ↗(On Diff #53238)

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

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)
Makefile
5 ↗(On Diff #53267)

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 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)

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
This revision was automatically updated to reflect the committed changes.