You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by br...@apache.org on 2018/10/14 02:53:08 UTC
svn commit: r1843807 - /subversion/trunk/subversion/libsvn_subr/sysinfo.c
Author: brane
Date: Sun Oct 14 02:53:08 2018
New Revision: 1843807
URL: http://svn.apache.org/viewvc?rev=1843807&view=rev
Log:
Restrict the loaded-library list on Linux to shared libraries.
* subversion/libsvn_subr/sysinfo.c (linux_shared_libs):
Show only mapped files that start with an ELF header that has
the correct type.
Modified:
subversion/trunk/subversion/libsvn_subr/sysinfo.c
Modified: subversion/trunk/subversion/libsvn_subr/sysinfo.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/sysinfo.c?rev=1843807&r1=1843806&r2=1843807&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/sysinfo.c (original)
+++ subversion/trunk/subversion/libsvn_subr/sysinfo.c Sun Oct 14 02:53:08 2018
@@ -697,6 +697,8 @@ linux_shared_libs(apr_pool_t *pool)
while (!eof)
{
svn_stringbuf_t *line;
+ const unsigned char *map_start;
+ const unsigned char *map_end;
err = svn_stream_readline(stream, &line, "\n", &eof, pool);
if (err)
@@ -705,7 +707,28 @@ linux_shared_libs(apr_pool_t *pool)
return NULL;
}
- /* Find the permissions of the mapped region. */
+ /* Address: The mapped memory address range. */
+ {
+ const char *const limit = line->data + line->len;
+ const char *start;
+ apr_int64_t val;
+ char *end;
+
+ /* The start of the address range */
+ start = line->data;
+ val = apr_strtoi64(start, &end, 16);
+ if (end == start || end >= limit || *end != '-')
+ continue;
+ map_start = (const unsigned char*)val;
+
+ /* The end of the address range */
+ start = end + 1;
+ val = apr_strtoi64(start, &end, 16);
+ if (end == start || end >= limit || !svn_ctype_isspace(*end))
+ continue;
+ map_end = (const unsigned char*)val;
+ }
+
stringbuf_skip_whitespace_field(line); /* skip address */
/* Permissions: The memory region must be readable and executable. */
@@ -723,13 +746,39 @@ linux_shared_libs(apr_pool_t *pool)
stringbuf_skip_whitespace_field(line); /* skip inode */
- /* Record anything that looks like an absolute path.
+ /* Consider only things that look like absolute paths.
Files that were removed since the process was created (due to an
upgrade, for example) are marked as '(deleted)'. */
if (line->data[0] == '/')
{
svn_version_ext_loaded_lib_t *lib;
+ /* Read the ELF header at the mapping position to check if this is
+ a shared library. We only look at the ELF identification and the
+ type. The format is described here:
+ http://www.skyfree.org/linux/references/ELF_Format.pdf
+ */
+ if (map_end - map_start < 18 || memcmp(map_start, "\x7f" "ELF", 4))
+ continue;
+
+ /* The ELF Type is 0x0003 for shared object files. */
+ switch (map_start[5]) /* Data encoding */
+ {
+ case 1: /* Little-Endian */
+ if (map_start[16] != 3 || map_start[17] != 0)
+ continue;
+ break;
+
+ case 2: /* Big-Endian */
+ if (map_start[16] != 0 || map_start[17] != 3)
+ continue;
+ break;
+
+ default:
+ continue;
+ }
+
+ /* We've done our best to find a mapped shared library. */
if (!result)
{
result = apr_array_make(pool, 32, sizeof(*lib));
Re: svn commit: r1843807 -
/subversion/trunk/subversion/libsvn_subr/sysinfo.c
Posted by Branko Čibej <br...@apache.org>.
On 14.10.2018 15:49, Daniel Shahaf wrote:
> brane@apache.org wrote on Sun, 14 Oct 2018 02:53 +0000:
>> Author: brane
>> Date: Sun Oct 14 02:53:08 2018
>> New Revision: 1843807
>>
>> URL: http://svn.apache.org/viewvc?rev=1843807&view=rev
>> Log:
>> Restrict the loaded-library list on Linux to shared libraries.
>>
>> * subversion/libsvn_subr/sysinfo.c (linux_shared_libs):
>> Show only mapped files that start with an ELF header that has
>> the correct type.
> I wonder if this isn't a bit too much. I don't want to have to deal
> with bug reports about 'svn --version --verbose' returning an error
> because the format of /proc/$pid/maps changed, about shared libraries
> not being listed in the output because their ELF magic bytes are
> different, and so on.
If the format of /proc/$pid/maps changes, our parser will ignore things
it doesn't understand. The format would have to change in very
unfortunate ways to break the checks in the code; that's not likely to
happen. This pseudo-file is a published API, changing the format in a
non-backward-compatible way would break a lot of other things before
Subversion.
These ELF magic bytes (the identification and type) won't change. Or
rather, they're as likely to change as Linux is likely to stop using
ELF. When that happens, we're sure to know in advance.
Regarding errors ... we don't return errors from these functions, we
ignore them on purpose, precisely so that 'svn --version --verbose'
won't throw an error. It's conceivable that we get a segfault because
the addresses in the maps file are wrong ... but that's as likely as
getting a bad pointer from the equivalent Windows or macOS functions. I
wouldn't loose sleep over that.
> It's not clear to me what functionality we gain in exchange for this
> complexity and maintenance cost. mmap() things that aren't .so, or that
> are .so but were mapped manually as opposed to by the runtime linker,
> won't be listed?
There's no difference between "mapped manually" and "mapped by the
runtime linker." The files on disk start with the ELF header, and that's
just mapped into memory. I would like to avoid is showing things like
cached JIT-compiled code and similar things that have nothing to do with
Subversion.
-- Brane
Re: svn commit: r1843807 -
/subversion/trunk/subversion/libsvn_subr/sysinfo.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
brane@apache.org wrote on Sun, 14 Oct 2018 02:53 +0000:
> Author: brane
> Date: Sun Oct 14 02:53:08 2018
> New Revision: 1843807
>
> URL: http://svn.apache.org/viewvc?rev=1843807&view=rev
> Log:
> Restrict the loaded-library list on Linux to shared libraries.
>
> * subversion/libsvn_subr/sysinfo.c (linux_shared_libs):
> Show only mapped files that start with an ELF header that has
> the correct type.
I wonder if this isn't a bit too much. I don't want to have to deal
with bug reports about 'svn --version --verbose' returning an error
because the format of /proc/$pid/maps changed, about shared libraries
not being listed in the output because their ELF magic bytes are
different, and so on.
It's not clear to me what functionality we gain in exchange for this
complexity and maintenance cost. mmap() things that aren't .so, or that
are .so but were mapped manually as opposed to by the runtime linker,
won't be listed?
Re: svn commit: r1843807 -
/subversion/trunk/subversion/libsvn_subr/sysinfo.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
brane@apache.org wrote on Sun, 14 Oct 2018 02:53 +0000:
> Author: brane
> Date: Sun Oct 14 02:53:08 2018
> New Revision: 1843807
>
> URL: http://svn.apache.org/viewvc?rev=1843807&view=rev
> Log:
> Restrict the loaded-library list on Linux to shared libraries.
>
> * subversion/libsvn_subr/sysinfo.c (linux_shared_libs):
> Show only mapped files that start with an ELF header that has
> the correct type.
I wonder if this isn't a bit too much. I don't want to have to deal
with bug reports about 'svn --version --verbose' returning an error
because the format of /proc/$pid/maps changed, about shared libraries
not being listed in the output because their ELF magic bytes are
different, and so on.
It's not clear to me what functionality we gain in exchange for this
complexity and maintenance cost. mmap() things that aren't .so, or that
are .so but were mapped manually as opposed to by the runtime linker,
won't be listed?