Add pthread to lang/lua53

[russ.haley_gmail.com]

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

[russ.haley_gmail.com]

Removed commented out code.

[andrew_tao173.riddles.org.uk]

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

[mat]

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.

[russ.haley_gmail.com]

Removed whitespace changes, used recommended svn diff options.

[andrew_tao173.riddles.org.uk]

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]

This comment was removed by russ.haley_gmail.com.

[russ.haley_gmail.com]

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]

Removed -pthread options. Added -pthread to LIBS.

[andrew_tao173.riddles.org.uk]

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]

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?

[russ.haley_gmail.com]

Added comment noting bug.

[andrew_tao173.riddles.org.uk]

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]

Add PORTVERSION.

[russ.haley_gmail.com]

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.