In the systemd unit for FS/debian, if the WorkingDirectory is not set
then it defaults to '/'. This is fine for FS, and is a common and
expected chdir choice for daemons.
We had previously set this to /run/freeswitch. Due to Debian having
systemd-coredump(8) disabled, this was causing core files to be
written to /run/freeswitch, which is a bad place for them as it's
mounted on tmpfs. So in commit cd68e0f we changed this to the log
directory /var/log/freeswitch. That choice is a bit usual as well on
Debian.
It's better to leave FS as running on '/'. This will prevent core
files from being written unless the user adjusts `sysctl
kernel.core_pattern`, which is a reasonable thing to expect if the
user wants these files. Core files can be huge, and even having them
go to /var/log unexpectedly can be a problem.
When Debian adds the systemd-coredumps support this will all work
nicely and automatically.
ref: http://www.freedesktop.org/software/systemd/man/coredump.conf.html
ref: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=744964
References-commit: cd68e0fe1b
FS-7909
Doing a chown of something in /etc/ as part of a systemd service file
is totally broken. It's far too large a sledgehammer to point at /etc
here. Someone may legitimately not be using /etc/freeswitch/tls in
his configuration, in which case this chown would fail and cause FS to
fail to start. Or someone may legitimately need /etc/freeswitch/tls
to have different ownership, in which case we would clobber it here.
The right thing to do is to create this directory in the
postinst (which we already are, assuming there is not an existing
configuration) and then perform the chown of it at the same time.
FS-7697
This changes how we create the temporary directory /run/freeswitch
when starting FS with systemd. The /run directory starts empty after
a system reboot, so we need to ensure this directory is present.
Originally for the FS systemd unit we created /run/freeswitch using
ExecStartPre and mkdir. With commit adb5f0d this was changed to use
the tmpfiles.d(5) mechanism (but the ExecStartPre/mkdir code was not
removed).
As part of systemd.exec(5), systemd provides a mechanism to
automatically manage these directories under /run and to bind their
lifetime to the lifetime of the process. This is actually what we
want here as compared to the more general tmpfiles.d(5) mechanism.
This commit moves to using the RuntimeDirectory= mechanism and removes
the obsolete ExecStartPre/mkdir code.
References-commit: adb5f0d278
Commit e5d247d (packaging python-esl) added trailing whitespace to the
package definition. Our trivial parser in the bootstrap script is
sensitive to this, so the change rather broke debian/bootstrap.sh.
This fixes commit e5d247df29.
When the debian/control-modules file is properly ordered and
formatted, running (cd debian && ./bootstrap.sh) will result in no
changes to the debian/control-modules file and the
debian/control-modules.gen file being automatically deleted. If there
is a debian/control-modules.gen file there, it should be used as a
start for a new debian/control-modules file after cleaning up the the
descriptions and adding long descriptions for each item.
The correct incantations to enable certification common name / subject
alternative name verification, per our code, are `subjects_all`,
`subjects_in`, and `subjects_out` in a Sofia profile's
`tls-verify-policy`. What we've had in our examples and documentation
for years are `all_subjects`, `in_subjects`, and `out_subjects`.
The result of this is that we've almost certainly confused people into
using the incorrect forms. Those poor people will believe that they
are verifying the CN/SAN of the received host certificate against the
list in `tls-verify-in-subjects` when in fact they are not.
One clear issue in this case was that the incorrect forms failed to
have any effect without providing any warning or error. This issue
could not have persisted if we had made more noise about incorrect
input.
Given how long this has been broken, it's tempting to alias the
incorrect forms to the correct ones. However this would certainly
break many existing installations that have, because of this error,
never actually tested their setup with CN/SAN validation enabled.
In this commit, we fix the examples and documentation, and add an
error-level log output when unknown values are passed to
`tls-verify-policy`.
Thanks-to: Andrew Patrikalakis <anrp+freeswitch@anrp.net>
Previously data returned from `memcache get` would be truncated at the
first NULL byte.
By using raw_write_function here to stream the returned memcache
value, we allow mod_memcache to be used for audio and other arbitrary
binary data.
Dave has a format module planned that relies on this.
Thanks-to: Dave Olszewski <dolszewski@marchex.com>
FS-7114 #resolve
The clamp(v,a,b) function wraps v around the interval [a,b).
However prior to this commit, `clamp` was ignoring the third argument
and using the second argument again in its place. This resulted in a
division by zero. Hence `clamp` didn't work at all.
Even if the arguments were treated correctly, `clamp` incorrectly
multiplied rather than added whenever v < a. This would have produced
bogus results. (Thanks to Shona McNeill for pointing this out.)
Note that as implemented, `clamp` is undefined for b >= a.
These errors are present in the last upstream C version, v2.7.
They've been corrected in the C++ version that upstream now maintains
instead.
Thanks-to: Shona McNeill <prufrax@googlemail.com>
FS-7070 #resolve
With this commit we allow getting `make -jN` behavior by setting
`parallel=N` in `DEB_BUILD_OPTIONS`. Setting `parallel=` will result
in `make -j` being run (i.e. no limit on the number of jobs). The
default is to run one job at a time; this is still the safest bet when
building all modules.
Thanks to Peter Wu for suggesting a different way to do this.
ref: https://www.debian.org/doc/debian-policy/ch-source.html#s-debianrules-options
Previously the `timeout` option to the curl API command set only
`CURLOPT_CONNECTTIMEOUT` -- the maximum amount of time that curl will
wait to connect to the server. If the server accepted the connection
but then never replied, curl would wait essentially forever. There
was no way to set `CURLOPT_TIMEOUT` -- the maximum amount of time the
entire request operation is allowed to take.
With this change, the `timeout` option sets `CURLOPT_TIMEOUT`. We've
earlier added a `connect-timeout` option to set
`CURLOPT_CONNECTTIMEOUT`.
This is a change to existing behavior. However, it's likely that this
is what people expected it to do all along. The curl application
call, for example, accepts both `curl_connect_timeout` and
`curl_timeout` channel variables, with the latter setting
`CURLOPT_TIMEOUT`.
If people really were relying on this odd behavior, we'll rename the
option with the new behavior to something else and come up with a
transition plan.
The libtool binary used to be contained in the libtool package. As of
current sid/jessie the libtool package contains everything about
libtool but the libtool binary, and we need to install libtool-bin to
get the libtool binary itself. Which is to say that Debian decided to
make the libtool package architecture-independent.
Without this commit the FS debian packaging will not build/bootstrap
on sid/jessie.
We use the transport of the Contact header of the remote UAC to decide
which of our own Contact addresses we should use when replying to a
SUBSCRIBE or sending a presence NOTIFY.
If TLS is not enabled on a Sofia profile, then the TLS Contacts for
that profile are NULL. Unfortunately we were using these NULL values
uncritically when the remote UAC sent us a Contact header with a TLS
transport and our own Sofia profile did not have TLS enabled.
With this commit we fall back to our TCP Contact address when the
remote Contact is TLS and our Sofia profile does not have TLS enabled.
This is a convenience option that can be passed to either
debian/bootstrap.sh or to debian/util.sh to include a module in the
build that would normally be excluded. This option, -p, can be passed
multiple times.
If -t is passed to debian/util.sh, then we will import the
/etc/apt/sources.list mirrors and all apt keys into the
pbuilder/cowbuilder build environment.
This is useful e.g. when the FS build depends on a library not
available in Debian yet.
Normally we override the config of each pbuilder/cowbuilder image when
we update the image before using it for the build. Passing this
option allows for using an existing cowbuilder image (assuming it's in
the correct location) with some existing desirable configuration.
At least some versions of GCC appear to take any -Wno-foo option
without error even if that option is not supported. But they will
error out if -Wno-error=foo is used. This sounds like a GCC bug, but
we'll work around it and test for the feature both ways.
Thanks-to: Matteo Brancaleoni <mbrancaleoni@voismart.it>
FS-6850 #resolve
With this commit, passing -Q to fs_cli will cause fs_cli to not load
or save the command history file (~/.fs_cli_history). This can be
useful e.g. on read-only systems.
This can also be set in the fs_cli configuration file with the option
`no-history-file`.
The debian/control-modules file is a comprehensive list of modules,
not a list of modules which are built. Our build tools verify the
file is comprehensive, so modules can't be removed from here unless
we're taking them out of tree.
Partially revert "don't package mod_cluechoo and don't build/load by default"
This partially reverts commit e02e41a5c1.
Revert "depend on fs before install"
This reverts commit 6c52217920.
Revert "removing commented work in progress on SDES and logging tunning on"
This reverts commit 6df5288f5a.
Revert "more formatting and logging tuning"
This reverts commit 0e89bbd033.
Revert "logging adjustment"
This reverts commit 764faad671.
Revert "missing host to network conversion highest_sequence_number_received"
This reverts commit 50c62cdfd7.
Revert "logging correction"
This reverts commit ea973b0b4c.
Revert "[FS-6623] implement RTCP report generation"
This reverts commit 0b7863a9b7.
When we specifically release all limits on a channel we destroy the
hash table stored in the "limit_hash" private channel data but we
don't destroy the private data as it will be reclaimed as part of the
session. If limit increment is called after the limit release we can
reuse that channel private, but we need to check whether the hash
table is null first. Fortunately this makes the code look better
anyway.
FS-6775 #resolve
FS-6783 #resolve
In `sofia status gateway ...` let's show the uptime in seconds rather
than in microseconds. We'll output the uptime in microseconds in
`xmlstatus` and we'll label it as such.
What if the hash were destroyed by one thread holding the lock while
another thread was waiting for the lock? The waiting thread would
have already checked that the hash was non-null and would proceed on
that assumption after acquiring the lock.
With this commit we check only after acquiring the lock.
ref: FS-6783
ref: FS-6775
`switch_core_hash_first` allocates an iterator on each call that is
never freed except when the hash table is empty.
By using `switch_core_hash_first_iter` we allocate only one iterator,
and that iterator is freed after the last item is processed.
We're using -Wno-unused-result to work around what is arguably a GCC
bug, but this option is not supported on older GCC versions. So on
those versions we won't pass -Werror either.
ref: commit b874048efc
It's reasonable for someone to change one or more of these directory
permissions after installation. We shouldn't touch more than we need
on upgrade. Each directory needs to be owned by the freeswitch user,
but past that we can leave discretion to the system administrator.
In some configurations where many limits are used, the limit increases
and releases were generating a lot of noise and making it harder to
see when limits were exceeded, which was at the same log level.
As an example of using mod_sofia's gateway parameter `contact-params`
we'd used the value `tport=tcp`. Looking around, it's clear this has
misled people into believing you can specify `tport=tcp` to make the
gateway use TCP or `tport=tls` to make the gateway use TLS. This does
not work.
The actual contact parameter is named `transport` rather than `tport`,
and you shouldn't use `transport` in `contact-params` because we
automatically add a `transport` to the Contact: based on the value of
`register-transport` (even if the gateway is set to not register).
It's clear why this would be confusing, so we'll just remove this as
an example.
In some cases where `redisplay()` is called immediately after a
command is run (e.g. `log ...`) we often get a prompt, junk output,
and a second prompt. This is due to a (known) race.
We believe we're falling afoul of this code in `el_deletestr`:
if (el->el_line.cursor < &el->el_line.buffer[n])
return;
Basing the length of text to delete off of the cursor position
resolves the issue of junk text, but the real solution is to eliminate
the race conditions, which will also resolve the sometimes duplicated
prompt.
FS-6764 #resolve
Thanks-to: Nathan Neulinger <nneul@neulinger.org>
Modern static analyzers warn when a variable is set but not used. GCC
warns when the result of a function marked as
`__attribute__((warn_unused_result))` is not set to some variable,
even when the function is cast to `(void)`, which is the recognized
way of indicating the intent to ignore the result. We treat all
warnings as errors.
The combination of behaviors here makes it difficult if we really
don't care about the result of the function; the obvious workarounds
are precluded.
GCC's maintainers don't consider this to be a bug:
Why do you think this is a bug? warn_unused_result is for cases where
"not checking the result is either a security problem or always a
bug".
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509#c1
GLIBC however marks functions like `read` and `write` with this
attribute. While it certainly in most cases is correct to act on
their return value, in some cases we really just don't care.
So when we see that we're building with GCC, and that we're building
with all warnings enabled, we'll just pass -Wno-unused-result to
disable the warning.
ref: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509
ref: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Clang's static analyzer thinks we could be using `hosts` here when it
is NULL. We probably weren't, but it's easy to see how it could think
so. We were checking whether `from_addr` matched `ice->addr` three
times, and between the second on third time we might have modified the
`ice->addr`; however we only get there if it matched the second time,
so we could only make it not match at that point and avoid the third
branch. We can't make it match where it did not before.
We'll simplify the logic a bit here so static analyzers (and humans)
can hopefully see this more readily.
When zero was passed for the size to `sub_alloc`, we were passing this
size on to `malloc` or `calloc`, which is unusual enough that static
analyzers warn about this (POSIX says that either NULL or a pointer
will be returned).
We'll instead just return NULL right away.
When the number of channels was zero, we were calling malloc with a
size of zero. While defined, this is unusual. And since we're
initializing the speex resampler as though there were one channel in
this case, we should probably just allocate the space for it.
Clang's static analyzer noticed the result of realloc was being
assigned to a pointer of a different type than was used to calculate
the new size. We can make things simpler and more idiomatic here by
using the correct pointer type and letting C's pointer arithmetic
automatically handle some multiplication.
We also use the distributive property here to simplify the calculation
for memset.
The `switch_split_date` and `switch_split_time` functions only set as
many variables as they believe exist values in the input string.
Since we didn't have defaults assigned we would read undefined stack
memory if the input string didn't contain e.g. an hour.
With this commit, we use 1970 if no year is present, January if no
month is present, the first day of the month if none is given, and
zero for each of a missing hour, minute, or second.
We're moving the variable declarations down to the smallest possible
scope (a good idea in general) so we can address the use of undefined
memory by the function in a later commit.
mod_curl currently does not verify the authenticity of the peer's
certificate, and does not verify whether the common name on the
certificate matches the server. This makes mod_curl initiated TLS
connections completely insecure. We should fix this, but until we do,
we'll warn people that it's not doing what they may think it is.
ref: http://curl.haxx.se/libcurl/c/CURLOPT_SSL_VERIFYPEER.html
ref: http://curl.haxx.se/libcurl/c/CURLOPT_SSL_VERIFYHOST.html
`curl_sendfile` generates a multipart message with Content-Type:
multipart/form-data with no separate Content-Type headers in the parts
for each non-file argument. These parts therefore default to
text/plain. However, prior to this commit, we were putting the URL
encoded POST data into these parts, which is not correct. We should
be putting raw text into the parts.
With this commit, we urldecode each argument key and value before
composing the multipart message.
See:
http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2
This prevents e.g. `git log -p` from showing the content changes in
these files by default, which is what we want because line-oriented
diffs of these files are meaningless.
Debian has packaged libpq version 9.4beta2. We were parsing the minor
version as "4beta2" and then trying to treat it as a constant integer,
which obviously doesn't work.
There's not really a perfect way to handle versions like this, so
we'll just pretend it's 9.4.0.
mod_prefix is an in-memory data store optimized for fast lookups
according to the longest prefix match (LPM) rule.
Tables of key-value string pairs in JSON format can be loaded at
startup via configuration and at runtime via the API.
The implementation uses a bitwise trie (aka binary prefix tree), so
arbitrary string keys are supported.
Travis Cross