Re: [VOTE] Switch read/write repository from Subversion to Git
On Tue, May 09, 2023 at 11:11:40PM -0500, Greg Stein wrote: > The ASF is completely confident in svn, and basically 99% of our corporate > records, and some of our key workflows (eg. account requests, TLP > graduation, ICLA recording) is all based on Apache Subversion. Also a fact. > And zero plans to change that. Git is not envisioned to replace any of that. > > Your implication that the Foundation was somehow required to grow the svn > community is misplaced. That is not its purpose. The Apache Subversion > community is responsible for growing itself. In 2011, the Board of > Directors specifically declined to assist a TLP with its community (*way* > larger than svn) because that is not the purpose of the Foundation. We do > not want some people "over there" on the Foundation/administrative side > interfering with the technical operation, and the community dynamics of one > of the communities. Or, even worse, to *pick* which communities get > assistance, while others do not. No winners. No losers. (clearly: I am > upset by your implication that you've been let down; the true answer is > missing a step on the Foundation's role) Thanks for writing this up, Greg. Point taken. I did not mean to imply that the foundation was somehow responsible for adding developers to the project. I did have some hope that we would see individual self-motivated contributors arriving via various ASF projects because they are all using SVN every day on svn.apache.org, are programmers, might have itches to scratch, already have commit access to ^/subversion, and there is some sense of shared ownership across the ASF community. I was reminded of all this by Graham's remark. It's the lack of such interactions that I find disappointing in retrospect. There certainly have been some, but relatively few. Of course, it's not the foundation's job to make that happen. It's up to the individuals in the larger ASF community to make a decision whether to get involved. > As Daniel notes else-thread, the suggestion is not git vs. svn. It is > entirely about "Do we want the tools offered by GitHub, to be made > available to the Apache HTTPD community?" Yep, that is very clear. And there is the network effect which makes GitHub a popular platform in its own right. > I'm an svn partisan, but I also appreciate GitHub for its utility. I > voted +1 to move, for that reason only. Hands-down, I'd -1 a move to git. > It was only GitHub that changed my opinion on this issue. I'll abstain since I don't contribute much to HTTPD anymore and probably won't find the time to do so in the foreseeable future. I'll run with whatever gets decided by the community. Cheers, Stefan
Re: [VOTE] Switch read/write repository from Subversion to Git
On Mon, May 08, 2023 at 10:29:04AM +0100, Graham Leggett via dev wrote: > This is a vote of no confidence in our own projects. If the ASF at large was confident in SVN as a technology then our current reality would look quite different. The Subversion project never managed to grow its developer base by becoming part of the ASF. We've mostly seen people who were already involved beforehand slowly disappearing over time. As a result, maintenance and development activity has declined sharply in recent years. Nobody at the ASF seems to notice or care and the result of a vote such as this won't make any difference in the matter. The overall preference for Github on the HTTPD project makes sense to me and isn't unexpected at all. Subversion passes as a stellar open source project by how it is being run but it is failing to attract open source communities as users based on technical merits. Most of the remaining users are running their Subversion servers behind closed doors.
Re: httpd test framework svn repo borked?
On Thu, Jan 13, 2022 at 07:17:27PM -0600, William A Rowe Jr wrote: > Thanks Stefan, it's attempting [locally] to replace a file, which was > just created during the > checkout (which might even be open). Hmm. I assume SVN would close such files based on APR pool lifetime. Handling of the pristine installation step has been changed in SVN trunk: https://svn.apache.org/r1886490 In particular: """ On Windows, it uses the existing mechanism of installing the files by handle, without having to reopen them. """ If you are running 1.14 or an older release, perhaps try a client built from trunk to see if this issue has already been fixed for a future SVN 1.15 release? > So it looks like some hash > collision or other > duplication issue where there are two attempts to emplace the same > hash svn-base image, > and linux is letting this happen without complaint, while windows refuses. Does the same file content (ignoring expanded keywords, if any) exist at multiple paths? If so, such paths would naturally share the same pristine based on the hash of file contents.
Re: httpd test framework svn repo borked?
On Thu, Jan 13, 2022 at 03:14:55PM -0600, William A Rowe Jr wrote: > I'm trying to work out how trunk on httpd test framework is busted on > Windows, irrespective > of using an msys2, sfl-ubuntu, or older win32 native svn build. The > problem always devolves > to something like shown below; > > svn co -q -r 1896892 > https://svn.apache.org/repos/asf/httpd/test/framework/trunk > httpdtest-1896892 > svn: E13: Can't move > '/c/Users/wrowe/dev/oss-httpd-build/src/httpdtest-1896892/.svn/tmp/svn-yIVLxQ' > to > '/c/Users/wrowe/dev/oss-httpd-build/src/httpdtest-1896892/.svn/pristine/b4/b4648076d6b2a3dfab889875d892252d5ed2594c.svn-base': > Permission denied This has nothing to do with the server, it is some problem with the working copy which is being checked out. It looks as if MoveFileExW is failing with EACCESS, see here: https://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?view=annotate#l4529 Though depending on which version of svn you have installed, svn might be calling directly into APR instead and fail there. > make: *** [../mak/Makefile.download:36: httpdtest-1896892] Error 1 > > The file it is attempting to move to already exists (why?) Any > guidance is appreciated, since > it is painful to grab this on a native ubuntu vm and slough it back to > windows to be able > to run the framework on windows. >
Re: Migrate to git?
On Mon, Oct 14, 2019 at 03:27:31PM +0100, Joe Orton wrote: > At the moment I think we have a quality control problem for 2.4.x, yet I > find it hard to justify spending much time on writing test cases because > that stuff is run so rarely. How many tests proposed in 2.4.x STATUS > have had a full test suite run? I certainly don't always do it. But if > we get the tests running all the time automatically it's much easier to > see a return on investment for improving test coverage. I see there's already a buildbot job for httpd trunk: https://ci.apache.org/waterfall?tag=httpd-trunk It seems this build job is configured to run a compile but it does not run the test suite? Putting Github/Travis questions aside, an easy way to get automated tests going with minimal effort today could be to run the existing tests inside httpd's existing buildbot job. The Subversion project has been doing this for years. See https://subversion.apache.org/buildbot/all
Re: [PATCH] mod_proxy: fix build without APR threads
On Tue, Jan 22, 2019 at 10:49:27AM -0600, William A Rowe Jr wrote: > On Tue, Jan 22, 2019 at 10:30 AM Stefan Sperling wrote: > > > On Tue, Jan 08, 2019 at 03:46:48PM +0100, Stefan Sperling wrote: > > > mod_proxy fails to compile when APR doesn't have thread support. > > > I don't know if this is supposed to be a supported configuration, > > > but this problem did not exist with HTTPD 2.2; it showed up in 2.4. > > > > How early in 2.4? I believe no-threads/prefork/mod_proxy had been > a supported combo during 2.4.1+ lifetime. It should be corrected. I don't know when it started. I had been lazy and kept compiling my SVN dev builds with httpd 2.2 for ages. I only switched that setup to the 2.4.x line with 2.4.37, because I finally ran into enough trouble keeping my 2.2-based setup in working condition. > mod_proxy_balancer doesn't make any sense, of course, so if it > has similar issues, those aren't a worry. I patched mod_proxy_balancer since it had compilation failures in my no-threads configuration. Are you saying mod_proxy_balancer should not be compiled in the first place if threads are disabled in APR? > > The patch below adds checks for APR_HAS_THREADS and passes test > > > builds with both threaded and non-threaded APR from APR's trunk. > > > Is this the right approach? > > > > > > I also found that the http2 module won't build without thread support. > > > > This is rather known. Early in the mod_http2 adoption, it was decided > that it would not support mod_prefork. Since it only supports threaded > MPM's, that's fine, it can be disabled for no-threads. OK, that's fine as it is then. > > > I can simply omit http2 from my SVN dev builds, for now. But mod_proxy > > > is required by mod_dav_svn for SVN's write-through proxy feature. > > > > > > Any feedback on this? Should I just commit it if I don't hear anything? > > > > You can commit what you believe is the right fix to trunk, and propose > this for backport in STATUS for 2.4 branch for others to consider, test > and collect the 3 +1's needed for backporting. Will do. Thanks!
Re: [PATCH] mod_proxy: fix build without APR threads
On Tue, Jan 08, 2019 at 03:46:48PM +0100, Stefan Sperling wrote:
> mod_proxy fails to compile when APR doesn't have thread support.
> I don't know if this is supposed to be a supported configuration,
> but this problem did not exist with HTTPD 2.2; it showed up in 2.4.
>
> The patch below adds checks for APR_HAS_THREADS and passes test
> builds with both threaded and non-threaded APR from APR's trunk.
> Is this the right approach?
>
> I also found that the http2 module won't build without thread support.
> I can simply omit http2 from my SVN dev builds, for now. But mod_proxy
> is required by mod_dav_svn for SVN's write-through proxy feature.
Any feedback on this? Should I just commit it if I don't hear anything?
> Index: modules/proxy/mod_proxy.h
> ===
> --- modules/proxy/mod_proxy.h (revision 1850749)
> +++ modules/proxy/mod_proxy.h (working copy)
> @@ -472,7 +472,9 @@ struct proxy_worker {
> proxy_conn_pool *cp;/* Connection pool to use */
> proxy_worker_shared *s; /* Shared data */
> proxy_balancer *balancer; /* which balancer am I in? */
> +#if APR_HAS_THREADS
> apr_thread_mutex_t *tmutex; /* Thread lock for updating address cache */
> +#endif
> void*context; /* general purpose storage */
> ap_conf_vector_t *section_config; /* -section wherein defined */
> };
> @@ -528,8 +530,10 @@ struct proxy_balancer {
> proxy_hashes hash;
> apr_time_t wupdated;/* timestamp of last change to workers list
> */
> proxy_balancer_method *lbmethod;
> +#if APR_HAS_THREADS
> apr_global_mutex_t *gmutex; /* global lock for updating list of workers
> */
> apr_thread_mutex_t *tmutex; /* Thread lock for updating shm */
> +#endif
> proxy_server_conf *sconf;
> void*context;/* general purpose storage */
> proxy_balancer_shared *s;/* Shared data */
> Index: modules/proxy/mod_proxy_balancer.c
> ===
> --- modules/proxy/mod_proxy_balancer.c(revision 1850749)
> +++ modules/proxy/mod_proxy_balancer.c(working copy)
> @@ -346,6 +346,7 @@ static proxy_worker *find_best_worker(proxy_balanc
> proxy_worker *candidate = NULL;
> apr_status_t rv;
>
> +#if APR_HAS_THREADS
> if ((rv = PROXY_THREAD_LOCK(balancer)) != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01163)
>"%s: Lock failed for find_best_worker()",
> @@ -352,6 +353,7 @@ static proxy_worker *find_best_worker(proxy_balanc
>balancer->s->name);
> return NULL;
> }
> +#endif
>
> candidate = (*balancer->lbmethod->finder)(balancer, r);
>
> @@ -358,11 +360,13 @@ static proxy_worker *find_best_worker(proxy_balanc
> if (candidate)
> candidate->s->elected++;
>
> +#if APR_HAS_THREADS
> if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01164)
>"%s: Unlock failed for find_best_worker()",
>balancer->s->name);
> }
> +#endif
>
> if (candidate == NULL) {
> /* All the workers are in error state or disabled.
> @@ -492,11 +496,13 @@ static int proxy_balancer_pre_request(proxy_worker
> /* Step 2: Lock the LoadBalancer
> * XXX: perhaps we need the process lock here
> */
> +#if APR_HAS_THREADS
> if ((rv = PROXY_THREAD_LOCK(*balancer)) != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01166)
>"%s: Lock failed for pre_request",
> (*balancer)->s->name);
> return DECLINED;
> }
> +#endif
>
> /* Step 3: force recovery */
> force_recovery(*balancer, r->server);
> @@ -557,20 +563,24 @@ static int proxy_balancer_pre_request(proxy_worker
> ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01167)
>"%s: All workers are in error state for route
> (%s)",
>(*balancer)->s->name, route);
> +#if APR_HAS_THREADS
> if ((rv = PROXY_THREAD_UNLOCK(*balancer)) != APR_SUCCESS) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01168)
>"%s: Unlock failed for pre_request",
>(*balancer)->s->name);
> }
> +#endif
> return HTTP_SERVICE_UNAVAILABLE;
> }
> }
>
> +#if APR_HAS_TH
Re: Apache 0-day / apache-uaf / use after free bugs
On Tue, Jan 22, 2019 at 01:31:43PM +0100, Rainer Jung wrote: > Here's the response we have compiled from Daniel, Stefan and others: > > https://bz.apache.org/bugzilla/show_bug.cgi?id=63098 FYI, I have disabled pool debugging in OpenBSD's port of APR. We are now using Yann's patch to force the default allocator to call free(3) when APR pools are cleared: https://marc.info/?l=openbsd-ports-cvs=154815812713288=2 This change only affects OpenBSD -current. I do not plan to backport a patch to the OpenBSD 6.4 release. We have had no reports indicating that http2 was crashing on OpenBSD. The likely reason is that nobody is actually running such a setup. If people intend to run such a setup, they should use -current for now, or wait until OpenBSD 6.5 is released.
[PATCH] mod_proxy: fix build without APR threads
mod_proxy fails to compile when APR doesn't have thread support.
I don't know if this is supposed to be a supported configuration,
but this problem did not exist with HTTPD 2.2; it showed up in 2.4.
The patch below adds checks for APR_HAS_THREADS and passes test
builds with both threaded and non-threaded APR from APR's trunk.
Is this the right approach?
I also found that the http2 module won't build without thread support.
I can simply omit http2 from my SVN dev builds, for now. But mod_proxy
is required by mod_dav_svn for SVN's write-through proxy feature.
Index: modules/proxy/mod_proxy.h
===
--- modules/proxy/mod_proxy.h (revision 1850749)
+++ modules/proxy/mod_proxy.h (working copy)
@@ -472,7 +472,9 @@ struct proxy_worker {
proxy_conn_pool *cp;/* Connection pool to use */
proxy_worker_shared *s; /* Shared data */
proxy_balancer *balancer; /* which balancer am I in? */
+#if APR_HAS_THREADS
apr_thread_mutex_t *tmutex; /* Thread lock for updating address cache */
+#endif
void*context; /* general purpose storage */
ap_conf_vector_t *section_config; /* -section wherein defined */
};
@@ -528,8 +530,10 @@ struct proxy_balancer {
proxy_hashes hash;
apr_time_t wupdated;/* timestamp of last change to workers list */
proxy_balancer_method *lbmethod;
+#if APR_HAS_THREADS
apr_global_mutex_t *gmutex; /* global lock for updating list of workers */
apr_thread_mutex_t *tmutex; /* Thread lock for updating shm */
+#endif
proxy_server_conf *sconf;
void*context;/* general purpose storage */
proxy_balancer_shared *s;/* Shared data */
Index: modules/proxy/mod_proxy_balancer.c
===
--- modules/proxy/mod_proxy_balancer.c (revision 1850749)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -346,6 +346,7 @@ static proxy_worker *find_best_worker(proxy_balanc
proxy_worker *candidate = NULL;
apr_status_t rv;
+#if APR_HAS_THREADS
if ((rv = PROXY_THREAD_LOCK(balancer)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01163)
"%s: Lock failed for find_best_worker()",
@@ -352,6 +353,7 @@ static proxy_worker *find_best_worker(proxy_balanc
balancer->s->name);
return NULL;
}
+#endif
candidate = (*balancer->lbmethod->finder)(balancer, r);
@@ -358,11 +360,13 @@ static proxy_worker *find_best_worker(proxy_balanc
if (candidate)
candidate->s->elected++;
+#if APR_HAS_THREADS
if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01164)
"%s: Unlock failed for find_best_worker()",
balancer->s->name);
}
+#endif
if (candidate == NULL) {
/* All the workers are in error state or disabled.
@@ -492,11 +496,13 @@ static int proxy_balancer_pre_request(proxy_worker
/* Step 2: Lock the LoadBalancer
* XXX: perhaps we need the process lock here
*/
+#if APR_HAS_THREADS
if ((rv = PROXY_THREAD_LOCK(*balancer)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01166)
"%s: Lock failed for pre_request", (*balancer)->s->name);
return DECLINED;
}
+#endif
/* Step 3: force recovery */
force_recovery(*balancer, r->server);
@@ -557,20 +563,24 @@ static int proxy_balancer_pre_request(proxy_worker
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01167)
"%s: All workers are in error state for route (%s)",
(*balancer)->s->name, route);
+#if APR_HAS_THREADS
if ((rv = PROXY_THREAD_UNLOCK(*balancer)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01168)
"%s: Unlock failed for pre_request",
(*balancer)->s->name);
}
+#endif
return HTTP_SERVICE_UNAVAILABLE;
}
}
+#if APR_HAS_THREADS
if ((rv = PROXY_THREAD_UNLOCK(*balancer)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01169)
"%s: Unlock failed for pre_request",
(*balancer)->s->name);
}
+#endif
if (!*worker) {
runtime = find_best_worker(*balancer, r);
if (!runtime) {
@@ -644,6 +654,7 @@ static int proxy_balancer_post_request(proxy_worke
apr_status_t rv;
+#if APR_HAS_THREADS
if ((rv = PROXY_THREAD_LOCK(balancer)) != APR_SUCCESS) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01173)
"%s: Lock failed for post_request",
@@ -650,6 +661,7 @@ static int proxy_balancer_post_request(proxy_worke
[PATCH] fix test/mod_dialup.c for non-threaded APR
See https://svn.apache.org/r1663375 for a related fix by covener.
Which, by the way, should probably be backported to 2.4; I see a failure
on a buildbot which deliberately builds with non-threaded APR to ensure
that this configuration remains in SVN's test matrix:
https://ci.apache.org/builders/svn-bb-openbsd/builds/272/steps/Build/logs/stdio
This patch hasn't even been compile tested but looks obvious enough.
Fine to commit?
Index: modules/test/mod_dialup.c
===
--- modules/test/mod_dialup.c (revision 1850735)
+++ modules/test/mod_dialup.c (working copy)
@@ -107,7 +107,9 @@ dialup_callback(void *baton)
dialup_baton_t *db = (dialup_baton_t *)baton;
conn_rec *c = db->r->connection;
+#if APR_HAS_THREADS
apr_thread_mutex_lock(db->r->invoke_mtx);
+#endif
status = dialup_send_pulse(db);
@@ -115,7 +117,9 @@ dialup_callback(void *baton)
ap_mpm_register_timed_callback(apr_time_from_sec(1), dialup_callback,
baton);
}
else if (status == DONE) {
+#if APR_HAS_THREADS
apr_thread_mutex_unlock(db->r->invoke_mtx);
+#endif
ap_finalize_request_protocol(db->r);
ap_process_request_after_handler(db->r);
return;
@@ -127,7 +131,9 @@ dialup_callback(void *baton)
ap_die(status, db->r);
}
+#if APR_HAS_THREADS
apr_thread_mutex_unlock(db->r->invoke_mtx);
+#endif
ap_mpm_resume_suspended(c);
}
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 23, 2018 at 02:32:30PM +0100, Yann Ylavic wrote: > Thanks Stefan, I didn't notice before in your proposed patch, but it > looks like uint64_t casts should be apr_uint64_t too. > > Regards, > Yann. Right. I went ahead and fixed it in r1849630. Thanks, Stefan
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Wed, Dec 19, 2018 at 07:03:39PM +0100, Stefan Sperling wrote:
> On Wed, Dec 19, 2018 at 02:58:28PM +0100, Yann Ylavic wrote:
> > On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling wrote:
> > >
> > > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> > > > But yes, upcast is better, while at it I'd go for uint64_t...
> > >
> > > Like this?
> >
> > I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;)
> >
> > Thanks for taking care of this !
>
> Sure :)
Committed.
> Index: modules/filters/mod_deflate.c
> ===
> --- modules/filters/mod_deflate.c (revision 1849274)
> +++ modules/filters/mod_deflate.c (working copy)
> @@ -893,8 +893,10 @@ static apr_status_t deflate_out_filter(ap_filter_t
> f->c->bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
> - "Zlib: Compressed %ld to %ld : URL %s",
> - ctx->stream.total_in, ctx->stream.total_out,
> r->uri);
> + "Zlib: Compressed %" APR_UINT64_T_FMT
> + " to %" APR_UINT64_T_FMT " : URL %s",
> + (uint64_t)ctx->stream.total_in,
> + (uint64_t)ctx->stream.total_out, r->uri);
>
> /* leave notes for logging */
> if (c->note_input_name) {
> @@ -1438,9 +1440,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
> ctx->validation_buffer_length += valid;
>
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
> - "Zlib: Inflated %ld to %ld : URL %s",
> - ctx->stream.total_in, ctx->stream.total_out,
> - r->uri);
> + "Zlib: Inflated %" APR_UINT64_T_FMT
> + " to %" APR_UINT64_T_FMT " : URL %s",
> + (uint64_t)ctx->stream.total_in,
> + (uint64_t)ctx->stream.total_out, r->uri);
>
> consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
> UPDATE_CRC, ctx->proc_bb);
> @@ -1459,9 +1462,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
> if ((ctx->stream.total_out & 0x) != compLen) {
> inflateEnd(>stream);
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
> APLOGNO(01395)
> - "Zlib: Length %ld of inflated data
> does "
> - "not match expected value %ld",
> - ctx->stream.total_out, compLen);
> + "Zlib: Length %" APR_UINT64_T_FMT
> + " of inflated data does not match"
> + " expected value %ld",
> + (uint64_t)ctx->stream.total_out,
> compLen);
> return APR_EGENERAL;
> }
> }
> @@ -1628,8 +1632,10 @@ static apr_status_t inflate_out_filter(ap_filter_t
> */
> flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
> - "Zlib: Inflated %ld to %ld : URL %s",
> - ctx->stream.total_in, ctx->stream.total_out,
> r->uri);
> + "Zlib: Inflated %" APR_UINT64_T_FMT
> + " to %" APR_UINT64_T_FMT " : URL %s",
> + (uint64_t)ctx->stream.total_in,
> + (uint64_t)ctx->stream.total_out, r->uri);
>
> if (ctx->validation_buffer_length == VALIDATION_SIZE) {
> unsigned long compCRC, compLen;
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Wed, Dec 19, 2018 at 02:58:28PM +0100, Yann Ylavic wrote:
> On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling wrote:
> >
> > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> > > But yes, upcast is better, while at it I'd go for uint64_t...
> >
> > Like this?
>
> I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;)
>
> Thanks for taking care of this !
Sure :)
Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c (revision 1849274)
+++ modules/filters/mod_deflate.c (working copy)
@@ -893,8 +893,10 @@ static apr_status_t deflate_out_filter(ap_filter_t
f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
- "Zlib: Compressed %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out, r->uri);
+ "Zlib: Compressed %" APR_UINT64_T_FMT
+ " to %" APR_UINT64_T_FMT " : URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
/* leave notes for logging */
if (c->note_input_name) {
@@ -1438,9 +1440,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
ctx->validation_buffer_length += valid;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
- "Zlib: Inflated %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out,
- r->uri);
+ "Zlib: Inflated %" APR_UINT64_T_FMT
+ " to %" APR_UINT64_T_FMT " : URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
UPDATE_CRC, ctx->proc_bb);
@@ -1459,9 +1462,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
if ((ctx->stream.total_out & 0x) != compLen) {
inflateEnd(>stream);
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
APLOGNO(01395)
- "Zlib: Length %ld of inflated data does "
- "not match expected value %ld",
- ctx->stream.total_out, compLen);
+ "Zlib: Length %" APR_UINT64_T_FMT
+ " of inflated data does not match"
+ " expected value %ld",
+ (uint64_t)ctx->stream.total_out,
compLen);
return APR_EGENERAL;
}
}
@@ -1628,8 +1632,10 @@ static apr_status_t inflate_out_filter(ap_filter_t
*/
flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
- "Zlib: Inflated %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out, r->uri);
+ "Zlib: Inflated %" APR_UINT64_T_FMT
+ " to %" APR_UINT64_T_FMT " : URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
if (ctx->validation_buffer_length == VALIDATION_SIZE) {
unsigned long compCRC, compLen;
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> But yes, upcast is better, while at it I'd go for uint64_t...
Like this?
I've noticed that the same problem seems to exist in some other modules.
I'll send separate patches for those once this patch has settled.
Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c (revision 1849274)
+++ modules/filters/mod_deflate.c (working copy)
@@ -893,8 +893,9 @@ static apr_status_t deflate_out_filter(ap_filter_t
f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
- "Zlib: Compressed %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out, r->uri);
+ "Zlib: Compressed %llu to %llu: URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
/* leave notes for logging */
if (c->note_input_name) {
@@ -1438,9 +1439,9 @@ static apr_status_t deflate_in_filter(ap_filter_t
ctx->validation_buffer_length += valid;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
- "Zlib: Inflated %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out,
- r->uri);
+ "Zlib: Inflated %llu to %llu : URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
UPDATE_CRC, ctx->proc_bb);
@@ -1459,9 +1460,9 @@ static apr_status_t deflate_in_filter(ap_filter_t
if ((ctx->stream.total_out & 0x) != compLen) {
inflateEnd(>stream);
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
APLOGNO(01395)
- "Zlib: Length %ld of inflated data does "
+ "Zlib: Length %llu of inflated data does
"
"not match expected value %ld",
- ctx->stream.total_out, compLen);
+ (uint64_t)ctx->stream.total_out,
compLen);
return APR_EGENERAL;
}
}
@@ -1628,8 +1629,9 @@ static apr_status_t inflate_out_filter(ap_filter_t
*/
flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
- "Zlib: Inflated %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out, r->uri);
+ "Zlib: Inflated %llu to %llu: URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
if (ctx->validation_buffer_length == VALIDATION_SIZE) {
unsigned long compCRC, compLen;
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 16, 2018 at 02:03:45PM +0100, Yann Ylavic wrote:
> On Sun, Dec 16, 2018 at 1:28 PM Stefan Sperling wrote:
> >
> > mod_deflates hard-codes some off_t format directives to "%ld".
> > It seems to me this code should use the macro provided by APR instead.
> >
> > Looking for another pair of eyes. Does this patch look good to commit?
>
> It seems that zlib defines total_in/out as uLong, i.e.:
> typedef struct z_stream_s {
> [...]
> uLongtotal_in; /* total number of input bytes read so far */
> [...]
> uLongtotal_out; /* total number of bytes output so far */
> [...]
> } z_stream;
>
> So %ld (or %lu) looks correct to me.
>
> (mod_brotli uses apr_off_t for them but it's an internal struct there).
>
> Regards,
> Yann.
Thanks for checking. This seems to depend on the version of zlib.
The file /usr/include/zlib.h I have on OpenBSD -current has this:
typedef struct z_stream_s {
[...]
z_off_t total_in; /* total nb of input bytes read so far */
[...]
z_off_t total_out; /* total nb of bytes output so far */
[...]
} z_stream;
And the compiler (clang) now rightly complains as follows:
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:856:27: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out, r->uri);
^~~~
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:856:49: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out, r->uri);
^
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1423:31: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out,
^~~~
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1423:53: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out,
^
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1450:39: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_out, compLen);
^
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1626:27: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out, r->uri);
^~~~
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1626:49: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out, r->uri);
^
7 errors generated.
This looks like an OpenBSD-specific change though (diff below).
I guess this will force OpenBSD to carry a local patch for
mod_deflate then, or just compile without -Werror.
RCS file: /cvs/src/lib/libz/zlib.h,v
Working file: zlib.h
head: 1.10
[...]
revision 1.7
date: 2003/12/16 22:35:50; author: henning; state: Exp; lines: +2 -2;
total_in and total_out need to be off_t, not unsigned long.
some bugs return: i fixed the same some months ago when we had this other gzip
there.
this bug resulted in wrong size stats for > 4GB files, and in the case
that the input file was > 4GB and could be compressed to < 4GB gzip
not zipping it as it would grow in its eyes.
=
Index: zlib.h
===
[PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
mod_deflates hard-codes some off_t format directives to "%ld".
It seems to me this code should use the macro provided by APR instead.
Looking for another pair of eyes. Does this patch look good to commit?
Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c (revision 1849023)
+++ modules/filters/mod_deflate.c (working copy)
@@ -893,7 +893,8 @@ static apr_status_t deflate_out_filter(ap_filter_t
f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
- "Zlib: Compressed %ld to %ld : URL %s",
+ "Zlib: Compressed %" APR_OFF_T_FMT
+ " to %" APR_OFF_T_FMT " : URL %s",
ctx->stream.total_in, ctx->stream.total_out, r->uri);
/* leave notes for logging */
@@ -1438,7 +1439,8 @@ static apr_status_t deflate_in_filter(ap_filter_t
ctx->validation_buffer_length += valid;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
- "Zlib: Inflated %ld to %ld : URL %s",
+ "Zlib: Inflated %" APR_OFF_T_FMT
+ " to %" APR_OFF_T_FMT " : URL %s",
ctx->stream.total_in, ctx->stream.total_out,
r->uri);
@@ -1459,7 +1461,8 @@ static apr_status_t deflate_in_filter(ap_filter_t
if ((ctx->stream.total_out & 0x) != compLen) {
inflateEnd(>stream);
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
APLOGNO(01395)
- "Zlib: Length %ld of inflated data does "
+ "Zlib: Length %" APR_OFF_T_FMT
+ " of inflated data does "
"not match expected value %ld",
ctx->stream.total_out, compLen);
return APR_EGENERAL;
@@ -1628,7 +1631,8 @@ static apr_status_t inflate_out_filter(ap_filter_t
*/
flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
- "Zlib: Inflated %ld to %ld : URL %s",
+ "Zlib: Inflated %" APR_OFF_T_FMT
+ " to %" APR_OFF_T_FMT " : URL %s",
ctx->stream.total_in, ctx->stream.total_out, r->uri);
if (ctx->validation_buffer_length == VALIDATION_SIZE) {
Re: Using APR pools "better"
On Wed, Sep 26, 2018 at 04:15:19PM -0500, Greg Stein wrote: > iterpool, scratch_pool, and result_pool are the KEY three concepts that we > learned while working on Subversion. Here's a recent example of where and why we added an iterpool (which should have been added when this loop was written in the first place): Log message: http://svn.apache.org/viewvc?view=revisionrevision=1841725 Diff: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/conflict-callbacks.c?r1=1841725=1841724=1841725 There is a key difference between Subversion and HTTP which affects pool lifetime concerns: SVN (on the client side) is a blocking application, while HTTPD is event-driven. Pools in HTTPD are often bound to the lifetime of a HTTP request (event) which means they are short-lived by design. Global data stored in pools is of a relatively fixed size. Whereas Subversion's client library is often used in a long-running process context (such the Windows Explorer in case of TortoiseSVN). The svn command line client alternates between blocking for user input and calling into the client library. The client library will return results to these API users and it has no idea about the lifetimes its callers require. Which is why letting the caller manage pool lifetime is a natural thing to do for this application. If you are going to write some general pool usage recommendations for APR I suppose such docs should mention how the various pool usage models relate to the software architectures they were invented for.
Re: Licensing claims (pcreposix)
On Tue, Feb 20, 2018 at 03:27:57PM -0600, William A Rowe Jr wrote: > I ran into the same headache with my complete rewrite of > the fnmatch.c logic of BSD that we ship in APR, and delivered > my rewrite of the file under both licenses. For which OpenBSD is still grateful, by the way :)
httpd 2.2 does not build without APR_HAS_THREADS
r1750836 broke the httpd 2.2 build if APR_HAS_THREADS is not defined.
I suppose this won't be fixed because 2.2 is EOL.
I just wanted to mention it in case somebody cares.
modules/proxy/proxy_util.c:1705: undefined reference to `socket_cleanup'
modules/proxy/.libs/libmod_proxy.a(proxy_util.o): In function
`ap_proxy_ssl_connection_cleanup':
modules/proxy/proxy_util.c:1743: undefined reference to `socket_cleanup'
modules/proxy/.libs/libmod_proxy.a(proxy_util.o): In function
`ap_proxy_determine_connection':
modules/proxy/proxy_util.c:2235: undefined reference to `socket_cleanup'
modules/proxy/proxy_util.c:2281: undefined reference to `socket_cleanup'
modules/proxy/proxy_util.c:2337: undefined reference to `socket_cleanup'
modules/proxy/.libs/libmod_proxy.a(proxy_util.o):modules/proxy/proxy_util.c:2542:
more undefined references to `socket_cleanup' follow
r1750836 | wrowe | 2016-06-30 19:07:29 +0200 (Thu, 30 Jun 2016) | 8 lines
mod_proxy: don't recyle backend announced "Connection: close" connections
to avoid reusing it should the close be effective after some new request
is ready to be sent.
Backports: r1678763, r1703807, r1703813, r1678763
Submitted by: ylavic
Reviewed by: rpluem, wrowe
Index: httpd/httpd/branches/2.2.x/STATUS
===
--- httpd/httpd/branches/2.2.x/STATUS (revision 1750835)
+++ httpd/httpd/branches/2.2.x/STATUS (revision 1750836)
@@ -138,18 +138,7 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
2.2.x patch: trunk works, modulo CHANGES
+1: rjung, wrowe, ylavic
- *) mod_proxy: don't recyle backend announced "Connection: close" connections
- to avoid reusing it should the close be effective after some new request
- is ready to be sent.
- trunk patch: http://svn.apache.org/r1678763
- http://svn.apache.org/r1703807
- http://svn.apache.org/r1703813
- 2.2.x patch:
http://home.apache.org/~ylavic/patches/httpd-2.2.x-mod_proxy-connection_close.patch
- +1: ylavic, rpluem, wrowe
- ylavic: while at it, I also included r1678763 which is only an
- optimization, but allows to keep code in sync with 2.4/trunk.
-
PATCHES PROPOSED TO BACKPORT FROM TRUNK:
[ New proposals should be added at the end of the list ]
Index: httpd/httpd/branches/2.2.x/CHANGES
===
--- httpd/httpd/branches/2.2.x/CHANGES (revision 1750835)
+++ httpd/httpd/branches/2.2.x/CHANGES (revision 1750836)
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes with Apache 2.2.32
+ *) mod_proxy: don't recyle backend announced "Connection: close" connections
+ to avoid reusing it should the close be effective after some new request
+ is ready to be sent. [Yann Ylavic]
+
*) mod_substitute: Allow to configure the patterns merge order with the new
SubstituteInheritBefore on|off directive. PR 57641
[Marc.Stern , Yann Ylavic, William Rowe]
Index: httpd/httpd/branches/2.2.x/modules/proxy/proxy_util.c
===
--- httpd/httpd/branches/2.2.x/modules/proxy/proxy_util.c (revision
1750835)
+++ httpd/httpd/branches/2.2.x/modules/proxy/proxy_util.c (revision
1750836)
@@ -1399,6 +1399,14 @@ PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker(
}
#if APR_HAS_THREADS
+static void socket_cleanup(proxy_conn_rec *conn)
+{
+conn->sock = NULL;
+conn->connection = NULL;
+conn->ssl_hostname = NULL;
+apr_pool_clear(conn->scpool);
+}
+
static apr_status_t conn_pool_cleanup(void *theworker)
{
proxy_worker *worker = (proxy_worker *)theworker;
@@ -1681,7 +1689,8 @@ static apr_status_t connection_cleanup(void *theco
#endif
/* determine if the connection need to be closed */
-if (!ap_proxy_connection_reusable(conn)) {
+if (!worker->is_address_reusable || worker->disablereuse
+|| conn->close_on_recycle) {
apr_pool_t *p = conn->pool;
apr_pool_clear(p);
conn = apr_pcalloc(p, sizeof(proxy_conn_rec));
@@ -1690,6 +1699,12 @@ static apr_status_t connection_cleanup(void *theco
apr_pool_create(&(conn->scpool), p);
apr_pool_tag(conn->scpool, "proxy_conn_scpool");
}
+else if (conn->close
+|| (conn->connection
+&& conn->connection->keepalive == AP_CONN_CLOSE)) {
+socket_cleanup(conn);
+conn->close = 0;
+}
#if APR_HAS_THREADS
if (worker->hmax && worker->cp->res) {
conn->inreslist = 1;
@@ -1705,14 +1720,6 @@ static apr_status_t connection_cleanup(void *theco
return APR_SUCCESS;
}
-static void socket_cleanup(proxy_conn_rec *conn)
-{
-conn->sock = NULL;
-conn->connection = NULL;
-conn->ssl_hostname = NULL;
-apr_pool_clear(conn->scpool);
-}
-
win32: disable shared LDAP cache by default
There are numerous reports of Apache HTTPD looping forever on Windows unless the LDAPSharedCacheSize option is set to zero. See for instance: https://svn.haxx.se/users/archive-2014-05/.shtml https://subversion.open.collab.net/ds/viewMessage.do?dsMessageId=564176=3 https://subversion.open.collab.net/ds/viewMessage.do?dsForumId=3=browseAll=539507 https://stackoverflow.com/questions/44542654/collabnet-subversion-server-reaching-cpu-100-because-of-httpd-exe-process I looked around for a while but don't know yet if a corresponding issue in the HTTPD bug tracker exists. Does anyone know? On the surface it looks like a memory pool corruption bug to me. The stack trace posted in https://svn.haxx.se/users/archive-2014-05/.shtml points towards an endless loop in apr_pool_cleanup_kill(). The trace ends at APR-util's misc/apr_reslist.c:apr_reslist_cleanup_order_set, and of the functions this calls only apr_pool_cleanup_kill() contains loops. I could not do any further debugging since I only had a production setup to look at, which is stable with the workaround 'LDAPSharedCacheSize 0'. I also do not have a Windows dev environment and I don't plan on digging any further. Until the real bug gets found and fixed, I would recommend making the known workaround the default on Windows. Because the winnt MPM runs a single process, there is no benefit to a shared memory cache anyway. Should I commit this patch? Index: modules/ldap/util_ldap.c === --- modules/ldap/util_ldap.c(revision 1803972) +++ modules/ldap/util_ldap.c(working copy) @@ -2815,7 +2815,17 @@ static void *util_ldap_create_config(apr_pool_t *p apr_thread_mutex_create(>mutex, APR_THREAD_MUTEX_DEFAULT, st->pool); #endif +#ifdef WIN32 +/* XXX The shared memory cache can cause an endless loop on Windows. + * See https://svn.haxx.se/users/archive-2014-05/.shtml and + * similar reports elsewhere which recommend 'LDAPSharedCacheSize 0' + * as a workaround. + * Because the winnt MPM uses a single process a shared cache is + * not needed anyway so leave it disabled by default. */ +st->cache_bytes = 0; +#else st->cache_bytes = 50; +#endif st->search_cache_ttl = 6; st->search_cache_size = 1024; st->compare_cache_ttl = 6;
Re: Alternate versioning proposal: patch line releases
On Thu, Jan 19, 2017 at 03:49:14PM -0800, Jacob Champion wrote: > We branch off from the 2.4.25 tag. I am not sure you mean this literally, but anyway: While basing a branch off of a tag (svn copy ^/tags/foo ^/branches/newbranch) works, I would recommend to always create a branch first, and then copy that branch to a tag. And then use this branch as destination for future patches, and as the copy source of tags representing future releases. So you would perform svn copies like this: svn copy ^/trunk ^/branches/2.4.x svn copy ^/branches/2.4.x ^/branches/2.4.25.x svn copy ^/branches/2.4.25.x ^/tags/2.4.25.0 # work on 2.4.25.x branch svn copy ^/branches/2.4.25.x ^/tags/2.4.25.1 # work on 2.4.25.x branch etc. The reason is that this way, all tags have copyfrom info pointing back to the branch they are derived from, and there is no special case where a tag suddenly becomes the copy source of a branch. This choice affects both the repository structure as well as the arguments people would be passing to svn copy to perform one of these steps. That said, this is mosly a cosmetic concern. The other way *should* work since SVN is always just dealing with copies. But I'm a tiny bit annoyed that SVN allows it :)
Re: mod_lets-encrypt
On Sat, Jan 14, 2017 at 07:15:29PM +0100, Dirk-Willem van Gulik wrote: > In fact - that may be a nice feature - an, essential, empheral port. Would that work for web servers behind firewalls?
Re: httpd and letsencrypt
On Fri, Aug 26, 2016 at 09:44:37AM -0700, Jacob Champion wrote: > On 08/26/2016 07:47 AM, Rich Bowen wrote: > >At LinuxCon I spoke with the director of the LetsEncrypt project - whose > >business card I haven't yet found in unpacking - and he asked whether > >the httpd project would be interested in LetsEncrypt being "in" httpd. > >That is, when one installs httpd, letsencrypt would just be a config > >option. (I have no idea how this would actually work, but that's beside > >the point really.) > > > >Is this something that we'd be interested in, if it were contributed? I > >note that their software is under the Apache License, so there shouldn't > >be any difficulty on that front. > > I assume you mean that they would donate a Let's Encrypt *client* for us to > ship? I think that would be neat. In case people are looking for a client implementation in C with a suitable licence, see https://kristaps.bsd.lv/letskencrypt/ Perhaps this code base can serve as a baseline or inspiration.
Re: patch (mod_ssl/ab) to support OPENSSL_NO_SSL3 builds
On Thu, Sep 10, 2015 at 10:37:44AM +, Stuart Henderson wrote:
> I've opened a ticket for this already (bz 58349) but it was suggested
> that I send mail here as well.
>
> Currently httpd builds fail with libressl as SSLv3 has been disabled
> (OPENSSL_NO_SSL3); ab.c and mod_ssl unconditionally use SSLv3_method()
> functions.
>
> ab.c fails at build time, mod_ssl is slightly nastier as this isn't
> picked up until trying to start a server with ssl enabled.
>
> Thanks,
> Stuart
Does OpenSSL use the name OPENSSL_NO_SSL3 too?
Or is this macro defined by LibreSSL only?
> --- support/ab.c.orig Fri Jul 17 22:55:57 2015
> +++ support/ab.c Fri Jul 17 22:56:13 2015
> @@ -2314,8 +2314,10 @@ int main(int argc, const char * const argv[])
> } else if (strncasecmp(opt_arg, "SSL2", 4) == 0) {
> meth = SSLv2_client_method();
> #endif
> +#ifndef OPENSSL_NO_SSL3
> } else if (strncasecmp(opt_arg, "SSL3", 4) == 0) {
> meth = SSLv3_client_method();
> +#endif
> #ifdef HAVE_TLSV1_X
> } else if (strncasecmp(opt_arg, "TLS1.1", 6) == 0) {
> meth = TLSv1_1_client_method();
>
> --- modules/ssl/ssl_engine_init.c.origSun Sep 6 15:23:52 2015
> +++ modules/ssl/ssl_engine_init.c Sun Sep 6 15:57:35 2015
> @@ -484,9 +484,15 @@ static apr_status_t ssl_init_ctx_protocol(server_rec *
> "Creating new SSL context (protocols: %s)", cp);
>
> if (protocol == SSL_PROTOCOL_SSLV3) {
> +#ifndef OPENSSL_NO_SSL3
> method = mctx->pkp ?
> SSLv3_client_method() : /* proxy */
> SSLv3_server_method(); /* server */
> +#else
> +ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s,
> +"SSLv3 protocol not available");
> +return ssl_die(s);
> +#endif
> }
> else if (protocol == SSL_PROTOCOL_TLSV1) {
> method = mctx->pkp ?
>
Re: svn commit: r1677149 - in /httpd/httpd/trunk/modules/ssl: ssl_util_ssl.c ssl_util_ssl.h
On Sat, May 02, 2015 at 11:10:50AM +0200, Kaspar Brand wrote: On 01.05.2015 16:29, s...@apache.org wrote: Author: stsp Date: Fri May 1 14:28:59 2015 New Revision: 1677149 URL: http://svn.apache.org/r1677149 Log: mod_ssl namespacing: Make SSL_ASN1_STRING_to_utf8 a static function inside ssl_util_ssl.c (no callers outside this file). The new static function name chosen is convert_asn1_to_utf8, based on the assumption that neither SSL_ nor ASN1_ are safe prefixes to use without potential future overlap. Thanks for pushing ahead with the namespace cleanup, Stefan. I'm fine with making the function static, but would suggest to rename it to asn1_string_to_utf8 instead - first, because it's really limited to ASN.1 strings (doesn't deal with arbitrary ASN.1 data), and second, because the _to_ already implies a conversion (plus it matches the naming of the other comparable functions in ssl_util_ssl.c). Kaspar Done: http://svn.apache.org/r1677339 I had minor reservations about using the asn1_ prefix since it is used inside of OpenSSL (though not in public APIs) and it is used in public APIs by GNU Libtasn1, used by GnuTLS. But it seems unnecessary to avoid a clash with asn1_ because mod_ssl's is a static function and mod_ssl won't compile with GnuTLS anyway.
mod_ssl: inline SSL_X509_INFO_load_path(); please review
I believe SSL_X509_INFO_load_path() should be inlined into
its only caller. I'd like some eyes on this change since
it's not just mechanical.
The desired behaviour seems to be load as many certs as possible
from a directory, looping over its file entries. Ignore errors,
e.g. in case the file is not a cert. The replaced function returned
a boolean which was never checked.
Regarding the removed comment about merging the dir-read loop
with another one: I don't think that's worth it.
Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c (revision 1677159)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -1247,7 +1247,26 @@ static apr_status_t ssl_init_proxy_certs(server_re
}
if (pkp-cert_path) {
-SSL_X509_INFO_load_path(ptemp, sk, pkp-cert_path);
+apr_dir_t *dir;
+apr_finfo_t dirent;
+apr_int32_t finfo_flags = APR_FINFO_TYPE|APR_FINFO_NAME;
+
+if (apr_dir_open(dir, pkp-cert_path, ptemp) == APR_SUCCESS) {
+while ((apr_dir_read(dirent, finfo_flags, dir)) == APR_SUCCESS) {
+const char *fullname;
+
+if (dirent.filetype == APR_DIR) {
+continue; /* don't try to load directories */
+}
+
+fullname = apr_pstrcat(ptemp,
+ pkp-cert_path, /, dirent.name,
+ NULL);
+modssl_X509_INFO_load_file(ptemp, sk, fullname);
+}
+
+apr_dir_close(dir);
+}
}
if ((ncerts = sk_X509_INFO_num(sk)) = 0) {
Index: modules/ssl/ssl_util_ssl.c
===
--- modules/ssl/ssl_util_ssl.c (revision 1677159)
+++ modules/ssl/ssl_util_ssl.c (working copy)
@@ -441,43 +441,6 @@ BOOL modssl_X509_INFO_load_file(apr_pool_t *ptemp,
return TRUE;
}
-BOOL SSL_X509_INFO_load_path(apr_pool_t *ptemp,
- STACK_OF(X509_INFO) *sk,
- const char *pathname)
-{
-/* XXX: this dir read code is exactly the same as that in
- * ssl_engine_init.c, only the call to handle the fullname is different,
- * should fold the duplication.
- */
-apr_dir_t *dir;
-apr_finfo_t dirent;
-apr_int32_t finfo_flags = APR_FINFO_TYPE|APR_FINFO_NAME;
-const char *fullname;
-BOOL ok = FALSE;
-
-if (apr_dir_open(dir, pathname, ptemp) != APR_SUCCESS) {
-return FALSE;
-}
-
-while ((apr_dir_read(dirent, finfo_flags, dir)) == APR_SUCCESS) {
-if (dirent.filetype == APR_DIR) {
-continue; /* don't try to load directories */
-}
-
-fullname = apr_pstrcat(ptemp,
- pathname, /, dirent.name,
- NULL);
-
-if (modssl_X509_INFO_load_file(ptemp, sk, fullname)) {
-ok = TRUE;
-}
-}
-
-apr_dir_close(dir);
-
-return ok;
-}
-
/* _
**
** Custom (EC)DH parameter support
Index: modules/ssl/ssl_util_ssl.h
===
--- modules/ssl/ssl_util_ssl.h (revision 1677159)
+++ modules/ssl/ssl_util_ssl.h (working copy)
@@ -68,7 +68,6 @@ char *modssl_X509_NAME_to_string(apr_pool_t
BOOLmodssl_X509_getSAN(apr_pool_t *, X509 *, int, int,
apr_array_header_t **);
BOOLmodssl_X509_match_name(apr_pool_t *, X509 *, const char *, BOOL,
server_rec *);
BOOLmodssl_X509_INFO_load_file(apr_pool_t *, STACK_OF(X509_INFO) *,
const char *);
-BOOLSSL_X509_INFO_load_path(apr_pool_t *, STACK_OF(X509_INFO) *, const
char *);
int SSL_CTX_use_certificate_chain(SSL_CTX *, char *, int,
pem_password_cb *);
char *SSL_SESSION_id2sz(unsigned char *, int, char *, int);
Re: mod_ssl namespacing: app_data2
On Fri, May 01, 2015 at 09:39:14AM -0400, Eric Covener wrote: On Fri, May 1, 2015 at 9:33 AM, Stefan Sperling s...@apache.org wrote: This moves symbols related to '2nd application data' into the ssl_ namespace. File-level static symbols have no external linkage so don't need a namespace. Same patch as before, but moving into modssl_ function namespace, instead of ssl_ which is used for private functions within OpenSSL. Hi Stefan -- I think CTR should be fine on these refactorings. So modssl_ has been agreed on? :) I'll go ahead. Thanks.
Re: mod_ssl namespacing: app_data2
On Sat, Apr 18, 2015 at 07:22:06PM +0200, Stefan Sperling wrote:
This moves symbols related to '2nd application data' into the ssl_ namespace.
File-level static symbols have no external linkage so don't need a namespace.
Same patch as before, but moving into modssl_ function namespace,
instead of ssl_ which is used for private functions within OpenSSL.
Index: modules/ssl/README.dsov.fig
===
--- modules/ssl/README.dsov.fig (revision 1677132)
+++ modules/ssl/README.dsov.fig (working copy)
@@ -339,7 +339,7 @@ Single
4 0 0 200 0 20 8 0. 4 90 465 11745 4770 -method\001
4 0 0 200 0 20 8 0. 4 120 1665 9945 6480 X509_STORE_CTX_get_app_data()\001
4 0 0 200 0 20 8 0. 4 120 1215 10980 6705 SSL_CTX_get_cert_store()\001
-4 0 0 200 0 20 8 0. 4 120 1020 8280 5130 SSL_get_app_data2()\001
+4 0 0 200 0 20 8 0. 4 120 1020 8280 5130 modssl_get_app_data2()\001
4 0 0 100 0 18 20 0. 4 270 1290 10710 7605 OpenSSL\001
4 0 0 100 0 18 12 0. 4 180 720 10710 7785 [Crypto]\001
4 0 0 100 0 18 20 0. 4 270 1290 10935 3645 OpenSSL\001
Index: modules/ssl/README.dsov.ps
===
--- modules/ssl/README.dsov.ps (revision 1677132)
+++ modules/ssl/README.dsov.ps (working copy)
@@ -1002,7 +1002,7 @@ gs 1 -1 sc (X509_STORE_CTX_get_app_data\(\)) col0
gs 1 -1 sc (SSL_CTX_get_cert_store\(\)) col0 sh gr
/Helvetica-Narrow-iso ff 120.00 scf sf
8280 5130 m
-gs 1 -1 sc (SSL_get_app_data2\(\)) col0 sh gr
+gs 1 -1 sc (modssl_get_app_data2\(\)) col0 sh gr
/Helvetica-Bold-iso ff 180.00 scf sf
3645 1620 m
gs 1 -1 sc (SSLDirConfig) col0 sh gr
Index: modules/ssl/mod_ssl.c
===
--- modules/ssl/mod_ssl.c (revision 1677132)
+++ modules/ssl/mod_ssl.c (working copy)
@@ -539,7 +539,7 @@ int ssl_init_ssl_connection(conn_rec *c, request_r
}
SSL_set_app_data(ssl, c);
-SSL_set_app_data2(ssl, NULL); /* will be request_rec */
+modssl_set_app_data2(ssl, NULL); /* will be request_rec */
sslconn-ssl = ssl;
Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c (revision 1677132)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -348,7 +348,7 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_po
*/
ssl_add_version_components(p, base_server);
-SSL_init_app_data2_idx(); /* for SSL_get_app_data2() at request time */
+modssl_init_app_data2_idx(); /* for modssl_get_app_data2() at request time
*/
init_dh_params();
Index: modules/ssl/ssl_engine_kernel.c
===
--- modules/ssl/ssl_engine_kernel.c (revision 1677132)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -229,7 +229,7 @@ int ssl_hook_ReadReq(request_rec *r)
}
}
#endif
-SSL_set_app_data2(ssl, r);
+modssl_set_app_data2(ssl, r);
/*
* Log information about incoming HTTPS requests
@@ -1385,7 +1385,7 @@ int ssl_callback_SSLVerify(int ok, X509_STORE_CTX
SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
conn_rec *conn = (conn_rec *)SSL_get_app_data(ssl);
-request_rec *r = (request_rec *)SSL_get_app_data2(ssl);
+request_rec *r = (request_rec *)modssl_get_app_data2(ssl);
server_rec *s = r ? r-server : mySrvFromConn(conn);
SSLSrvConfigRec *sc = mySrvConfig(s);
Index: modules/ssl/ssl_util_ssl.c
===
--- modules/ssl/ssl_util_ssl.c (revision 1677132)
+++ modules/ssl/ssl_util_ssl.c (working copy)
@@ -38,19 +38,19 @@
* also note that OpenSSL increments at static variable when
* SSL_get_ex_new_index() is called, so we _must_ do this at startup.
*/
-static int SSL_app_data2_idx = -1;
+static int app_data2_idx = -1;
-void SSL_init_app_data2_idx(void)
+void modssl_init_app_data2_idx(void)
{
int i;
-if (SSL_app_data2_idx -1) {
+if (app_data2_idx -1) {
return;
}
/* we _do_ need to call this twice */
-for (i=0; i=1; i++) {
-SSL_app_data2_idx =
+for (i = 0; i = 1; i++) {
+app_data2_idx =
SSL_get_ex_new_index(0,
Second Application Data for SSL,
NULL, NULL, NULL);
@@ -57,14 +57,14 @@
}
}
-void *SSL_get_app_data2(SSL *ssl)
+void *modssl_get_app_data2(SSL *ssl)
{
-return (void *)SSL_get_ex_data(ssl, SSL_app_data2_idx);
+return (void *)SSL_get_ex_data(ssl, app_data2_idx);
}
-void SSL_set_app_data2(SSL *ssl, void *arg)
+void modssl_set_app_data2(SSL *ssl, void *arg)
{
-SSL_set_ex_data(ssl, SSL_app_data2_idx, (char *)arg);
+SSL_set_ex_data(ssl, app_data2_idx, (char *)arg);
return
Re: trunk and FreeBSD 10.1
On Thu, Apr 23, 2015 at 12:28:33PM -0400, Jim Jagielski wrote: I tried that, but gmake totally barfed... I have the same issue on OpenBSD. I think the GNU-BSD Makefile style transformation implemented by build/bsd_makefile is too simplistic. All it does it putting a dot in front of include statements: include - .include It doesn't seem to handle other differences between GNU make and BSD make. Cases such as CC_FOR_BUILD being set to an empty string: build/config_vars.mk:CC_FOR_BUILD = and then checked with ifdef and used if defined: server/Makefile.in:ifdef CC_FOR_BUILD server/Makefile.in: $(CC_FOR_BUILD) $(CFLAGS_FOR_BUILD) -DCROSS_COMPILE -o $ @ $ It's defined, but empty, so that's where the build failure comes from. I can only build httpd trunk on OpenBSD by first disabling the BSD Makefile transformation logic and then using gmake: ./configure --disable-bsd-makefiles gmake (Also make sure to revert any local .mk changes created by prior runs of the bsd_makefile script -- they upset gmake.) I don't think the current approach is a good one. Either write portable Makefiles, or decide on one particular implementation of make. The middle ground will tend to break down every now and then. Also, on BSD the local changes to .mk files in the working copy get in the way of 'svn diff' and 'svn commit'. In the SVN project we write portable Makefiles which work with either implementation. This has been working out quite well. On Apr 23, 2015, at 12:07 PM, Jeff Trawick traw...@gmail.com wrote: On 04/23/2015 11:59 AM, Jim Jagielski wrote: Hmmm... I am seeing some strangeness trying to get trunk to build on FreeBSD 10.1... I get to ./server/ and then the build dies w/ try gmake? IIRC I had noticed that some BSD make compatibility was lost at some point... -DCROSS_COMPILE -o gen_test_char make[2]: exec(-DCROSS_COMPILE) failed (No such file or directory) *** Error code 1 It looks like the Makefile.in-Makefile process is causing issues. This is what I get in the Makefile: .include $(top_builddir)/build/rules.mk .include $(top_srcdir)/build/library.mk .ifdef CC_FOR_BUILD gen_test_char: gen_test_char.c $(CC_FOR_BUILD) $(CFLAGS_FOR_BUILD) -DCROSS_COMPILE -o $@ $ .else gen_test_char_OBJECTS = gen_test_char.lo gen_test_char: $(gen_test_char_OBJECTS) $(LINK) $(EXTRA_LDFLAGS) $(gen_test_char_OBJECTS) $(EXTRA_LIBS) .endif So we can see where the error is... those shell vars aren't defined/found. But why?
Re: trunk and FreeBSD 10.1
On Fri, Apr 24, 2015 at 07:20:01AM -0400, Jim Jagielski wrote: There is also the magic that configure does at well, in altering the directives (#ifdef - .ifdef, for example). I think what I'll do is, if running under *BSD, see not only if 'make' itself is really GNUmake, but also check for 'gmake' and, if it exists, disable BSD makefiles. Or perhaps declare BSD make as unsupported and require GNU make. That's the easiest short-term solution in my opinion. The existing BSD make support doesn't work anyway and making it work is a larger effort.
Re: svn commit: r1674542 - in /httpd/httpd/trunk: acinclude.m4 modules/ssl/ssl_engine_rand.c
On Wed, Apr 22, 2015 at 09:29:49AM +0200, Kaspar Brand wrote:
Sorry for having missed this in my previous review: we should also
#ifdef the SSL_RSSRC_EGD case in
ssl_engine_config.c:ssl_cmd_SSLRandomSeed(), to make sure that egd:...
settings are not silently ignored when mod_ssl is compiled against
LibreSSL. Either let the failure then be detected by the
ssl_util_path_check, or (probably better) reject it similar to how it's
done for SSLCompression, SSLHonorCipherOrder etc.
Kaspar
Thanks, good catch.
Is this fine?
Index: modules/ssl/ssl_engine_config.c
===
--- modules/ssl/ssl_engine_config.c (revision 1675266)
+++ modules/ssl/ssl_engine_config.c (working copy)
@@ -574,8 +574,15 @@ const char *ssl_cmd_SSLRandomSeed(cmd_parms *cmd,
seed-cpPath = ap_server_root_relative(mc-pPool, arg2+5);
}
else if ((arg2len 4) strEQn(arg2, egd:, 4)) {
+#ifdef HAVE_RAND_EGD
seed-nSrc = SSL_RSSRC_EGD;
seed-cpPath = ap_server_root_relative(mc-pPool, arg2+4);
+#else
+return apr_pstrcat(cmd-pool, Invalid SSLRandomSeed entropy source `,
+ arg2, ': This version of MODSSL_LIBRARY_NAME
+does not support the Entropy Gathering Daemon
+ (EGD)., NULL);
+#endif
}
else if (strcEQ(arg2, builtin)) {
seed-nSrc = SSL_RSSRC_BUILTIN;
Re: namespacing in mod_ssl
On Wed, Apr 15, 2015 at 06:36:13PM +0200, Stefan Sperling wrote:
However, the actual issue here is that mod_ssl is squatting the SSL_
namespace.
Historically this may have made sense (it seems mod_ssl and OpenSSL have
shared history/authors). Bill Rowe suggested to try moving mod_ssl's
functions into the ap_ namespace to avoid such clashes in the future.
Given the feedback so far, it seems we all generally agree that
the current namespacing is a problem that needs to be addressed.
I like Jeff's suggestion to split this task up into smaller changes
by looking at each symbol on a case-by-case basis.
And the alternative naming schemes suggested make sense to me.
So to take a first step forward, here's a diff that puts ssl_util_ssl.h
macros into the MODSSL_ namespace.
Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c (revision 1674422)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -148,12 +148,12 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_po
apr_status_t rv;
apr_array_header_t *pphrases;
-if (SSLeay() SSL_LIBRARY_VERSION) {
+if (SSLeay() MODSSL_LIBRARY_VERSION) {
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, base_server, APLOGNO(01882)
Init: this version of mod_ssl was compiled against
a newer library (%s, version currently loaded is %s)
- may result in undefined or erroneous behavior,
- SSL_LIBRARY_TEXT, SSLeay_version(SSLEAY_VERSION));
+ MODSSL_LIBRARY_TEXT, SSLeay_version(SSLEAY_VERSION));
}
/* We initialize mc-pid per-process in the child init,
@@ -242,7 +242,7 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_po
#endif
ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(01883)
- Init: Initialized %s library, SSL_LIBRARY_NAME);
+ Init: Initialized %s library, MODSSL_LIBRARY_NAME);
/*
* Seed the Pseudo Random Number Generator (PRNG)
Index: modules/ssl/ssl_engine_io.c
===
--- modules/ssl/ssl_engine_io.c (revision 1674422)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2186,7 +2186,7 @@ long ssl_io_data_cb(BIO *bio, int cmd,
}
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s,
-SSL_LIBRARY_NAME,
+MODSSL_LIBRARY_NAME,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? write : read),
rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? to :
from),
bio, argp, dump);
@@ -2196,7 +2196,7 @@ long ssl_io_data_cb(BIO *bio, int cmd,
else {
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
%s: I/O error, %d bytes expected to %s on BIO#%pp [mem:
%pp],
-SSL_LIBRARY_NAME, argi,
+MODSSL_LIBRARY_NAME, argi,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? write : read),
bio, argp);
}
Index: modules/ssl/ssl_engine_kernel.c
===
--- modules/ssl/ssl_engine_kernel.c (revision 1674422)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -1654,7 +1654,7 @@ static void ssl_session_log(server_rec *s,
const char *result,
long timeout)
{
-char buf[SSL_SESSION_ID_STRING_LEN];
+char buf[MODSSL_SESSION_ID_STRING_LEN];
char timeout_str[56] = {'\0'};
if (!APLOGdebug(s)) {
@@ -1811,32 +1811,32 @@ static void log_tracing_state(const SSL *ssl, conn
*/
if (where SSL_CB_HANDSHAKE_START) {
ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, c,
- %s: Handshake: start, SSL_LIBRARY_NAME);
+ %s: Handshake: start, MODSSL_LIBRARY_NAME);
}
else if (where SSL_CB_HANDSHAKE_DONE) {
ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, c,
- %s: Handshake: done, SSL_LIBRARY_NAME);
+ %s: Handshake: done, MODSSL_LIBRARY_NAME);
}
else if (where SSL_CB_LOOP) {
ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, c,
%s: Loop: %s,
- SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
+ MODSSL_LIBRARY_NAME, SSL_state_string_long(ssl));
}
else if (where SSL_CB_READ) {
ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, c,
%s: Read: %s,
- SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
+ MODSSL_LIBRARY_NAME, SSL_state_string_long(ssl));
}
else if (where SSL_CB_WRITE) {
ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, c,
%s: Write: %s
mod_ssl namespacing: app_data2
This moves symbols related to '2nd application data' into the ssl_ namespace.
File-level static symbols have no external linkage so don't need a namespace.
Index: modules/ssl/README.dsov.fig
===
--- modules/ssl/README.dsov.fig (revision 1674542)
+++ modules/ssl/README.dsov.fig (working copy)
@@ -339,7 +339,7 @@ Single
4 0 0 200 0 20 8 0. 4 90 465 11745 4770 -method\001
4 0 0 200 0 20 8 0. 4 120 1665 9945 6480 X509_STORE_CTX_get_app_data()\001
4 0 0 200 0 20 8 0. 4 120 1215 10980 6705 SSL_CTX_get_cert_store()\001
-4 0 0 200 0 20 8 0. 4 120 1020 8280 5130 SSL_get_app_data2()\001
+4 0 0 200 0 20 8 0. 4 120 1020 8280 5130 ssl_get_app_data2()\001
4 0 0 100 0 18 20 0. 4 270 1290 10710 7605 OpenSSL\001
4 0 0 100 0 18 12 0. 4 180 720 10710 7785 [Crypto]\001
4 0 0 100 0 18 20 0. 4 270 1290 10935 3645 OpenSSL\001
Index: modules/ssl/README.dsov.ps
===
--- modules/ssl/README.dsov.ps (revision 1674542)
+++ modules/ssl/README.dsov.ps (working copy)
@@ -1002,7 +1002,7 @@ gs 1 -1 sc (X509_STORE_CTX_get_app_data\(\)) col0
gs 1 -1 sc (SSL_CTX_get_cert_store\(\)) col0 sh gr
/Helvetica-Narrow-iso ff 120.00 scf sf
8280 5130 m
-gs 1 -1 sc (SSL_get_app_data2\(\)) col0 sh gr
+gs 1 -1 sc (ssl_get_app_data2\(\)) col0 sh gr
/Helvetica-Bold-iso ff 180.00 scf sf
3645 1620 m
gs 1 -1 sc (SSLDirConfig) col0 sh gr
Index: modules/ssl/mod_ssl.c
===
--- modules/ssl/mod_ssl.c (revision 1674542)
+++ modules/ssl/mod_ssl.c (working copy)
@@ -570,7 +570,7 @@ int ssl_init_ssl_connection(conn_rec *c, request_r
}
SSL_set_app_data(ssl, c);
-SSL_set_app_data2(ssl, NULL); /* will be request_rec */
+ssl_set_app_data2(ssl, NULL); /* will be request_rec */
sslconn-ssl = ssl;
Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c (revision 1674542)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -348,7 +348,7 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_po
*/
ssl_add_version_components(p, base_server);
-SSL_init_app_data2_idx(); /* for SSL_get_app_data2() at request time */
+ssl_init_app_data2_idx(); /* for ssl_get_app_data2() at request time */
init_dh_params();
Index: modules/ssl/ssl_engine_kernel.c
===
--- modules/ssl/ssl_engine_kernel.c (revision 1674542)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -229,7 +229,7 @@ int ssl_hook_ReadReq(request_rec *r)
}
}
#endif
-SSL_set_app_data2(ssl, r);
+ssl_set_app_data2(ssl, r);
/*
* Log information about incoming HTTPS requests
@@ -1385,7 +1385,7 @@ int ssl_callback_SSLVerify(int ok, X509_STORE_CTX
SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
conn_rec *conn = (conn_rec *)SSL_get_app_data(ssl);
-request_rec *r = (request_rec *)SSL_get_app_data2(ssl);
+request_rec *r = (request_rec *)ssl_get_app_data2(ssl);
server_rec *s = r ? r-server : mySrvFromConn(conn);
SSLSrvConfigRec *sc = mySrvConfig(s);
Index: modules/ssl/ssl_util_ssl.c
===
--- modules/ssl/ssl_util_ssl.c (revision 1674542)
+++ modules/ssl/ssl_util_ssl.c (working copy)
@@ -38,19 +38,19 @@
* also note that OpenSSL increments at static variable when
* SSL_get_ex_new_index() is called, so we _must_ do this at startup.
*/
-static int SSL_app_data2_idx = -1;
+static int app_data2_idx = -1;
-void SSL_init_app_data2_idx(void)
+void ssl_init_app_data2_idx(void)
{
int i;
-if (SSL_app_data2_idx -1) {
+if (app_data2_idx -1) {
return;
}
/* we _do_ need to call this twice */
-for (i=0; i=1; i++) {
-SSL_app_data2_idx =
+for (i = 0; i = 1; i++) {
+app_data2_idx =
SSL_get_ex_new_index(0,
Second Application Data for SSL,
NULL, NULL, NULL);
@@ -57,14 +57,14 @@
}
}
-void *SSL_get_app_data2(SSL *ssl)
+void *ssl_get_app_data2(SSL *ssl)
{
-return (void *)SSL_get_ex_data(ssl, SSL_app_data2_idx);
+return (void *)SSL_get_ex_data(ssl, app_data2_idx);
}
-void SSL_set_app_data2(SSL *ssl, void *arg)
+void ssl_set_app_data2(SSL *ssl, void *arg)
{
-SSL_set_ex_data(ssl, SSL_app_data2_idx, (char *)arg);
+SSL_set_ex_data(ssl, app_data2_idx, (char *)arg);
return;
}
Index: modules/ssl/ssl_util_ssl.h
===
--- modules/ssl/ssl_util_ssl.h (revision 1674542)
+++ modules/ssl/ssl_util_ssl.h (working copy)
@@ -57,9 +57,9 @@
/**
*
Re: check for RAND_egd at configure time
On Fri, Apr 17, 2015 at 07:02:21AM +0200, Kaspar Brand wrote:
I was actually thinking about
Index: acinclude.m4
===
--- acinclude.m4(revision 1673835)
+++ acinclude.m4(working copy)
@@ -594,7 +594,7 @@
liberrors=
AC_CHECK_HEADERS([openssl/engine.h])
AC_CHECK_FUNCS([SSLeay_version SSL_CTX_new], [], [liberrors=yes])
- AC_CHECK_FUNCS([ENGINE_init ENGINE_load_builtin_engines])
+ AC_CHECK_FUNCS([ENGINE_init ENGINE_load_builtin_engines RAND_egd])
if test x$liberrors != x; then
AC_MSG_WARN([OpenSSL libraries are unusable])
fi
... or does that not fit in this case?
Kaspar
Thanks, this work fine. Tested on OpenBSD and Debian.
Index: acinclude.m4
===
--- acinclude.m4(revision 1673798)
+++ acinclude.m4(working copy)
@@ -594,7 +594,7 @@ AC_DEFUN(APACHE_CHECK_OPENSSL,[
liberrors=
AC_CHECK_HEADERS([openssl/engine.h])
AC_CHECK_FUNCS([SSLeay_version SSL_CTX_new], [], [liberrors=yes])
- AC_CHECK_FUNCS([ENGINE_init ENGINE_load_builtin_engines])
+ AC_CHECK_FUNCS([ENGINE_init ENGINE_load_builtin_engines RAND_egd])
if test x$liberrors != x; then
AC_MSG_WARN([OpenSSL libraries are unusable])
fi
Index: modules/ssl/ssl_engine_rand.c
===
--- modules/ssl/ssl_engine_rand.c (revision 1673798)
+++ modules/ssl/ssl_engine_rand.c (working copy)
@@ -86,6 +86,7 @@ int ssl_rand_seed(server_rec *s, apr_pool_t *p, ss
nDone += ssl_rand_feedfp(p, fp, pRandSeed-nBytes);
ssl_util_ppclose(s, p, fp);
}
+#ifdef HAVE_RAND_EGD
else if (pRandSeed-nSrc == SSL_RSSRC_EGD) {
/*
* seed in contents provided by the external
@@ -95,6 +96,7 @@ int ssl_rand_seed(server_rec *s, apr_pool_t *p, ss
continue;
nDone += n;
}
+#endif
else if (pRandSeed-nSrc == SSL_RSSRC_BUILTIN) {
struct {
time_t t;
Re: check for RAND_egd at configure time
On Wed, Apr 15, 2015 at 08:43:04PM +0200, Stefan Sperling wrote:
LibreSSL does not provide the RAND_egd() function.
This patch adds a configure check to allow building mod_ssl with LibreSSL.
Updated version following Kaspar Brand's suggestion to move into acinclude.m4.
Index: acinclude.m4
===
--- acinclude.m4(revision 1673798)
+++ acinclude.m4(working copy)
@@ -598,6 +598,11 @@ AC_DEFUN(APACHE_CHECK_OPENSSL,[
if test x$liberrors != x; then
AC_MSG_WARN([OpenSSL libraries are unusable])
fi
+ have_rand_egd=no
+ AC_CHECK_LIB(crypto, RAND_egd, [have_rand_egd=yes])
+ if test $have_rand_egd = yes; then
+AC_DEFINE([HAVE_RAND_EGD], [1], [Define if RAND_egd exists.])
+ fi
else
AC_MSG_WARN([OpenSSL version is too old])
fi
Index: modules/ssl/ssl_engine_rand.c
===
--- modules/ssl/ssl_engine_rand.c (revision 1673798)
+++ modules/ssl/ssl_engine_rand.c (working copy)
@@ -86,6 +86,7 @@ int ssl_rand_seed(server_rec *s, apr_pool_t *p, ss
nDone += ssl_rand_feedfp(p, fp, pRandSeed-nBytes);
ssl_util_ppclose(s, p, fp);
}
+#ifdef HAVE_RAND_EGD
else if (pRandSeed-nSrc == SSL_RSSRC_EGD) {
/*
* seed in contents provided by the external
@@ -95,6 +96,7 @@ int ssl_rand_seed(server_rec *s, apr_pool_t *p, ss
continue;
nDone += n;
}
+#endif
else if (pRandSeed-nSrc == SSL_RSSRC_BUILTIN) {
struct {
time_t t;
namespacing in mod_ssl
A few months ago, OpenBSD's LibreSSL added a new function
called SSL_CTX_use_certificate_chain().
This unexpectedly broke the build of Apache's mod_ssl which defines
a function of the same name. In OpenBSD this was worked around by
patching mod_ssl, renaming the clashing function.
Since then LibreSSL has renamed to SSL_CTX_use_certificate_chain_mem().
This resolved the immediate problem and mod_ssl patches could be dropped.
However, the actual issue here is that mod_ssl is squatting the SSL_ namespace.
Historically this may have made sense (it seems mod_ssl and OpenSSL have
shared history/authors). Bill Rowe suggested to try moving mod_ssl's
functions into the ap_ namespace to avoid such clashes in the future.
I have verified that no undefined references or undeclared symbols
are reported by gcc during the build with this diff.
I have tested startup of a httpd with mod_ssl loaded.
Additional testing is appreciated.
I'm not sure how README.dsov.ps should be regenerated.
It contains a refence to SSL_get_app_data2().
For now I've only updated the .dsov.fig file.
Index: modules/ssl/README.dsov.fig
===
--- modules/ssl/README.dsov.fig (revision 1673798)
+++ modules/ssl/README.dsov.fig (working copy)
@@ -339,7 +339,7 @@
4 0 0 200 0 20 8 0. 4 90 465 11745 4770 -method\001
4 0 0 200 0 20 8 0. 4 120 1665 9945 6480 X509_STORE_CTX_get_app_data()\001
4 0 0 200 0 20 8 0. 4 120 1215 10980 6705 SSL_CTX_get_cert_store()\001
-4 0 0 200 0 20 8 0. 4 120 1020 8280 5130 SSL_get_app_data2()\001
+4 0 0 200 0 20 8 0. 4 120 1020 8280 5130 ap_SSL_get_app_data2()\001
4 0 0 100 0 18 20 0. 4 270 1290 10710 7605 OpenSSL\001
4 0 0 100 0 18 12 0. 4 180 720 10710 7785 [Crypto]\001
4 0 0 100 0 18 20 0. 4 270 1290 10935 3645 OpenSSL\001
Index: modules/ssl/mod_ssl.c
===
--- modules/ssl/mod_ssl.c (revision 1673798)
+++ modules/ssl/mod_ssl.c (working copy)
@@ -570,7 +570,7 @@
}
SSL_set_app_data(ssl, c);
-SSL_set_app_data2(ssl, NULL); /* will be request_rec */
+ap_SSL_set_app_data2(ssl, NULL); /* will be request_rec */
sslconn-ssl = ssl;
Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c (revision 1673798)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -148,12 +148,12 @@
apr_status_t rv;
apr_array_header_t *pphrases;
-if (SSLeay() SSL_LIBRARY_VERSION) {
+if (SSLeay() AP_SSL_LIBRARY_VERSION) {
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, base_server, APLOGNO(01882)
Init: this version of mod_ssl was compiled against
a newer library (%s, version currently loaded is %s)
- may result in undefined or erroneous behavior,
- SSL_LIBRARY_TEXT, SSLeay_version(SSLEAY_VERSION));
+ AP_SSL_LIBRARY_TEXT, SSLeay_version(SSLEAY_VERSION));
}
/* We initialize mc-pid per-process in the child init,
@@ -242,7 +242,7 @@
#endif
ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(01883)
- Init: Initialized %s library, SSL_LIBRARY_NAME);
+ Init: Initialized %s library, AP_SSL_LIBRARY_NAME);
/*
* Seed the Pseudo Random Number Generator (PRNG)
@@ -348,7 +348,7 @@
*/
ssl_add_version_components(p, base_server);
-SSL_init_app_data2_idx(); /* for SSL_get_app_data2() at request time */
+ap_SSL_init_app_data2_idx(); /* for ap_SSL_get_app_data2() at request time
*/
init_dh_params();
@@ -871,9 +871,9 @@
}
}
-n = SSL_CTX_use_certificate_chain(mctx-ssl_ctx,
- (char *)chain,
- skip_first, NULL);
+n = ap_SSL_CTX_use_certificate_chain(mctx-ssl_ctx,
+ (char *)chain,
+ skip_first, NULL);
if (n 0) {
ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(01903)
Failed to configure CA certificate chain!);
@@ -946,7 +946,7 @@
* Some information about the certificate(s)
*/
-if (SSL_X509_getBC(cert, is_ca, pathlen)) {
+if (ap_SSL_X509_getBC(cert, is_ca, pathlen)) {
if (is_ca) {
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s, APLOGNO(01906)
%s server certificate is a CA certificate
@@ -961,8 +961,8 @@
}
}
-if (SSL_X509_match_name(ptemp, cert, (const char *)s-server_hostname,
-TRUE, s) == FALSE) {
+if (ap_SSL_X509_match_name(ptemp, cert, (const char *)s-server_hostname,
+ TRUE, s) == FALSE) {
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s, APLOGNO(01909)
%s server certificate
check for RAND_egd at configure time
LibreSSL does not provide the RAND_egd() function.
This patch adds a configure check to allow building mod_ssl with LibreSSL.
Index: modules/ssl/config.m4
===
--- modules/ssl/config.m4 (revision 1673798)
+++ modules/ssl/config.m4 (working copy)
@@ -44,6 +44,12 @@
# structure, so ask libtool to hide everything else:
APR_ADDTO(MOD_SSL_LDADD, [-export-symbols-regex ssl_module])
fi
+
+have_rand_egd=no
+AC_CHECK_LIB(crypto, RAND_egd, [have_rand_egd=yes])
+if test $have_rand_egd = yes; then
+AC_DEFINE([HAVE_RAND_EGD], [1], [Define if RAND_egd exists.])
+fi
else
enable_ssl=no
fi
Index: modules/ssl/ssl_engine_rand.c
===
--- modules/ssl/ssl_engine_rand.c (revision 1673798)
+++ modules/ssl/ssl_engine_rand.c (working copy)
@@ -86,6 +86,7 @@
nDone += ssl_rand_feedfp(p, fp, pRandSeed-nBytes);
ssl_util_ppclose(s, p, fp);
}
+#ifdef HAVE_RAND_EGD
else if (pRandSeed-nSrc == SSL_RSSRC_EGD) {
/*
* seed in contents provided by the external
@@ -95,6 +96,7 @@
continue;
nDone += n;
}
+#endif
else if (pRandSeed-nSrc == SSL_RSSRC_BUILTIN) {
struct {
time_t t;
unbreak mod_ssl build following removal of chil engine in LibreSSL
I'm trying to update OpenBSD's port of Apache HTTPD to the 2.4 series.
This is the first in a small series of diffs from the OpenBSD ports tree.
I hope OpenBSD will eventually be able to drop its custom patches. If you're
at ApacheCon in Austin this week please feel free to talk to me in person.
mod_ssl unconditionally uses a macro related to the chil engine, which has
been removed from LibreSSL. Guard use of this macro based on its presence.
Patch by Stuart Henderson, obtained by me from the OpenBSD ports tree.
http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/www/apache-httpd/patches/patch-modules_ssl_ssl_engine_init_c?rev=1.1content-type=text/x-cvsweb-markup
Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c (revision 1673296)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -374,9 +374,11 @@
return ssl_die(s);
}
+#ifdef ENGINE_CTRL_CHIL_SET_FORKCHECK
if (strEQ(mc-szCryptoDevice, chil)) {
ENGINE_ctrl(e, ENGINE_CTRL_CHIL_SET_FORKCHECK, 1, 0, 0);
}
+#endif
if (!ENGINE_set_default(e, ENGINE_METHOD_ALL)) {
ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(01889)
