Fix: Patch attached with submission follows:
Maintainer of net/freeradius2, Please note that PR ports/137425 has just been submitted. If it contains a patch for an upgrade, an enhancement or a bug fix you agree on, reply to this email stating that you approve the patch and a committer will take care of it. The full text of the PR can be found at: http://www.freebsd.org/cgi/query-pr.cgi?pr=ports/137425 -- Edwin Groothuis via the GNATS Auto Assign Tool edwin@FreeBSD.org
State Changed From-To: open->feedback Awaiting maintainers feedback (via the GNATS Auto Assign Tool)
Responsible Changed From-To: freebsd-ports-bugs->wxs I'll take it.
Thanks very for this, Ryan - others have mentioned Oracle support, but nobody has come up with a patch before, let alone a complete patch in a PR. There's a few things I'd like to sort out before this is committed. I think ORACLE is a better name for the option and plist variable than the rather cryptic OCI8. The (EXPERIMENTAL) label in an option means that the EXPERIMENTAL switch is implied by that option. I believe you are declaring Oracle support in FreeRADIUS on FreeBSD to be experimental, but that you don't need EXPERIMENTAL set. If so, I think we need to explain this a different way, rather than overloading the (EXPERIMENTAL) label. Can you explain the patch to src/main/exec.c? It would be good to know where it has come from (for example, is it mentioned in the documentation for rlm_sql_oracle, or on bugs.freeradius.org?) and why it is needed. I see what it does, but with FreeRADIUS being a security critical port, I'd rather understand it fully before this is committed. I've tried to limit patches in the FreeRADIUS ports to the build system only since I took over maintainership, but sometimes patches to the actual code are necessary, and this seems to be one of those occasions. The Debian OpenSSL debacle stands out in my mind as a demonstration of the danger of single line changes to security critical code, though I realise this patch doesn't touch cryptographic code. PORTREVISION should arguably be bumped. The patch as it stands is not approved, but I'm sure we can sort out the minor changes to get it into shape. I'm very grateful for your submission and look forward to devising an approved patch together. Unfortunately I'm away for most of the next two and a half weeks, but I'll try to keep an eye on this PR via my laptop, especially with the FreeBSD 8.0 ports freeze so close. With best wishes, David (maintainer, net/freeradius2) -- David Wood david@wood2.org.uk
David, I selected OCI8 as it appears to be what is used in other ports, specifically referencing the oracle 8 client that lives in ports. There is a version 7 of the client (databases/oracle7-client) so I specifically wanted to ensure we ended up with version 8 of the client. www/codeigniter and databases/p5-DBD-Oracle are examples of where WITH_OCI8 is used elsewhere within the ports tree. You are correct about the EXPERIMENTAL label meaning that oracle support under freebsd is experimental, not that we need the EXPERIMENTAL module set to use oracle. As for the patch to src/main/exec.c, the Oracle libraries appear to hijack various signals for some silly reason. The hijacking of the SIGCHLD signal breaks FR's ability to use exec as intended, which is required by the mschap module as well as the exec module. Without the patch, FR doesn't think that the process it exec'd completed successfully and, as you can imagine, this creates other issues. The Oracle documentation that I have come across in the past says "The pipe driver uses SIGCLD, also referred to as SIGCHLD, when an Oracle process dies." Since FR actually forks() to use exec, I believe this is where the Oracle libraries cause havoc for us. The patch simply resets the sighandler back to the default before forking, which (in my testing) has resolved the issue. Since it doesn't appear as if FR defines a special handler for SIGCHLD, I feel the default is appropriate. There may be a more appropriate way of dealing with this, but, this seems to work. Considering how minimal of a patch it is, I have decided to submit it as is. However, I'd love to hear/see an alternative. I have been running FR2 with the patch to exec.c (and with oracle support) for a few months now in a production environment on three separate hosts with great success. I have, however, seen FR segfault on exit (with the Oracle libraries loaded, which I believe happens whenever detach_modules() is called) and therefor have flagged Oracle support as being experimental. As far as PORTREVISION goes, I agree and apologize for missing that in my original patch. I will submit a new patch that addresses the EXPERIMENTAL label and PORTREVISION. Thanks, -r On (08/06/09 22:57), David Wood wrote: > Thanks very for this, Ryan - others have mentioned Oracle support, but > nobody has come up with a patch before, let alone a complete patch in a > PR. > > > There's a few things I'd like to sort out before this is committed. > > > I think ORACLE is a better name for the option and plist variable than > the rather cryptic OCI8. > > > The (EXPERIMENTAL) label in an option means that the EXPERIMENTAL switch > is implied by that option. I believe you are declaring Oracle support in > FreeRADIUS on FreeBSD to be experimental, but that you don't need > EXPERIMENTAL set. If so, I think we need to explain this a different > way, rather than overloading the (EXPERIMENTAL) label. > > > Can you explain the patch to src/main/exec.c? It would be good to know > where it has come from (for example, is it mentioned in the > documentation for rlm_sql_oracle, or on bugs.freeradius.org?) and why it > is needed. I see what it does, but with FreeRADIUS being a security > critical port, I'd rather understand it fully before this is committed. > > I've tried to limit patches in the FreeRADIUS ports to the build system > only since I took over maintainership, but sometimes patches to the > actual code are necessary, and this seems to be one of those occasions. > The Debian OpenSSL debacle stands out in my mind as a demonstration of > the danger of single line changes to security critical code, though I > realise this patch doesn't touch cryptographic code. > > > PORTREVISION should arguably be bumped. > > > The patch as it stands is not approved, but I'm sure we can sort out the > minor changes to get it into shape. I'm very grateful for your > submission and look forward to devising an approved patch together. > > > Unfortunately I'm away for most of the next two and a half weeks, but > I'll try to keep an eye on this PR via my laptop, especially with the > FreeBSD 8.0 ports freeze so close. > > > With best wishes, > > > > > David (maintainer, net/freeradius2) > -- > David Wood > david@wood2.org.uk -- Ryan Steinmetz Lead Security/Systems Administrator Infrastructure Engineering Rochester Institute of Technology 585.475.5663 PGP: EF36 D45A 5CA9 28B1 A550 18CD A43C D111 7AD7 FAF2
Updated patch attached. On (08/06/09 22:57), David Wood wrote: > Thanks very for this, Ryan - others have mentioned Oracle support, but > nobody has come up with a patch before, let alone a complete patch in a > PR. > > > There's a few things I'd like to sort out before this is committed. > > > I think ORACLE is a better name for the option and plist variable than > the rather cryptic OCI8. > > > The (EXPERIMENTAL) label in an option means that the EXPERIMENTAL switch > is implied by that option. I believe you are declaring Oracle support in > FreeRADIUS on FreeBSD to be experimental, but that you don't need > EXPERIMENTAL set. If so, I think we need to explain this a different > way, rather than overloading the (EXPERIMENTAL) label. > > > Can you explain the patch to src/main/exec.c? It would be good to know > where it has come from (for example, is it mentioned in the > documentation for rlm_sql_oracle, or on bugs.freeradius.org?) and why it > is needed. I see what it does, but with FreeRADIUS being a security > critical port, I'd rather understand it fully before this is committed. > > I've tried to limit patches in the FreeRADIUS ports to the build system > only since I took over maintainership, but sometimes patches to the > actual code are necessary, and this seems to be one of those occasions. > The Debian OpenSSL debacle stands out in my mind as a demonstration of > the danger of single line changes to security critical code, though I > realise this patch doesn't touch cryptographic code. > > > PORTREVISION should arguably be bumped. > > > The patch as it stands is not approved, but I'm sure we can sort out the > minor changes to get it into shape. I'm very grateful for your > submission and look forward to devising an approved patch together. > > > Unfortunately I'm away for most of the next two and a half weeks, but > I'll try to keep an eye on this PR via my laptop, especially with the > FreeBSD 8.0 ports freeze so close. > > > With best wishes, > > > > > David (maintainer, net/freeradius2) > -- > David Wood > david@wood2.org.uk -- Ryan Steinmetz Lead Security/Systems Administrator Infrastructure Engineering Rochester Institute of Technology 585.475.5663 PGP: EF36 D45A 5CA9 28B1 A550 18CD A43C D111 7AD7 FAF2
On Thu, Aug 06, 2009 at 10:57:11PM +0100, David Wood wrote: > PORTREVISION should arguably be bumped. I only did a quick review of the patch before Ryan submitted it and I thought about this. The option defaults to off so there is no need to bump PORTREVISION (it doesn't affect the package and it doesn't fix a bug in the existing port). I leave the rest of your comments up to Ryan but want to say thank you for being responsive and I look forward to concluding this matter. -- WXS
On Thu, Aug 06, 2009 at 11:30:07PM +0000, Ryan Steinmetz wrote: > Updated patch attached. David, Do you have any comments on this updated patch? I'd like to see PORTREVISION removed for the reason I mentioned earlier: the patch adds an option which defaults to off so it doesn't affect the default package or currently installed ports. This patch has been languishing since August 6th so I'd like to bring it to a conclusion and get it committed. If you are not comfortable approving the changes to the code I'd suggest we get one of the upstream developers to review the proposed changes. Thanks! -- WXS
David, I've added an updated patch that is up to date in light of the libtool/libltdl updates. I've also added a 'make config' knob for enabling developer mode, which can help developers more-easily debug things using the version of FR in ports. -r On (08/24/09 16:05), Wesley Shields wrote: > On Thu, Aug 06, 2009 at 11:30:07PM +0000, Ryan Steinmetz wrote: > > Updated patch attached. > > David, > > Do you have any comments on this updated patch? I'd like to see > PORTREVISION removed for the reason I mentioned earlier: the patch adds > an option which defaults to off so it doesn't affect the default package > or currently installed ports. > > This patch has been languishing since August 6th so I'd like to bring it > to a conclusion and get it committed. If you are not comfortable > approving the changes to the code I'd suggest we get one of the > upstream developers to review the proposed changes. > > Thanks! > > -- WXS -- Ryan Steinmetz Lead Security/Systems Administrator Infrastructure Engineering Rochester Institute of Technology 585.475.5663 PGP: EF36 D45A 5CA9 28B1 A550 18CD A43C D111 7AD7 FAF2
wxs 2009-09-09 17:02:31 UTC FreeBSD ports repository Modified files: net/freeradius2 Makefile pkg-plist Added files: net/freeradius2/files extra-patch-exec.c Log: - Add support for Oracle and debugging options (both off by default). PR: ports/137425 Submitted by: Ryan Steinmetz <rpsfa@rit.edu> Approved by: maintainer timeout Revision Changes Path 1.79 +19 -1 ports/net/freeradius2/Makefile 1.1 +11 -0 ports/net/freeradius2/files/extra-patch-exec.c (new) 1.40 +5 -0 ports/net/freeradius2/pkg-plist _______________________________________________ cvs-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/cvs-all To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
State Changed From-To: feedback->closed Committed. Thanks!