Hi ftpmaster,
Shayan Doust worked really hard to fix an issue in r-cran-rstan to
enable smooth building of r-cran-rstanarm. For the moment it is not
clear yet why this fix does not work for armhf, i386 and mips64el but
other architectures are building nicely. We will keep on investigating
this issue but it would be important to let r-cran-rstanarm migrate
to testing to let a whole bunch of other R package migrate as well.
So please remove r-cran-rstanarm for the said architectures from
unstable.
Thanks a lot for your ftpmaster work
Andreas.
Hi Andreas This are actually two bugs: - r-base dyn.load not accepting relative library names on Linux systems and - r-cran-rcppparallel trying to workaround the bug in dyn.load by deducting the full path of libtbb from the architecture instead of the correct multiarch setting and failing. This has nothing to do with r-cran-rstan or r-cran-rstanarm, but it seems to be the first one to find out. I've attached patches to fix both problems, properly re-assigned and adjusted the bugs. This behaviour of R dyn.load might even be considered a security vulnerability, because loading libraries from the working directory is a problem. Bastian
Hi Bastian, Thanks for taking the time to propose a bug report. However (and please see below) I don't think this is the way forward. There have been a lot of recent changed in RcppParallel upstream (as I lurk on the GH repo which is part of our Rcpp org), and maybe some of these things will be different under the as-of-right-now-still-unreleased version 5.1.0 of RcppParallel. I am CCing its (upstream) maintainer / core developer who is also a friend and Rcpp collaborator. In the meantime please see https://github.com/RcppCore/RcppParallel/blob/master/inst/NEWS As for your suggested patch to R's own dynload.c: that is very well tested and robust system code I do not have any real intention of changing because one package out of 17k at CRAN is having hickups under one (maybe suboptimal) Debian config. Maybe Andreas (for r-cran-rcppparallel) should talk to Kevin here to see what if anything could or should be changed. As for ~ expansion, we usually do that _before_ calling dyn.load() and passing paths on to other C level functions: > Sys.glob("~/.Rprofile") [1] "/home/edd/.Rprofile" > I would be happy to take a patch forward to R Core and fend for it, as we have in fact done in the past, but I think I would need to see a stronger case of why we would all of a sudden inject a getcwd() here. Happy to chat more and hear real arguments if there are any. Cheers, Dirk On 10 February 2021 at 18:55, Bastian Blank wrote: | Control: clone -1 -2 | Control: reassign -1 r-base 4.0.3-1 | Control: retitle -1 r-base: dyn.load not useful for system libraries | Control: affects -1 r-cran-rcppparallel 5.0.2+dfsg-3 | Control: severity -1 important | Control: reassign -2 r-cran-rcppparallel 5.0.2+dfsg-3 | Control: retitle -2 r-cran-rcppparallel: generates broken load path for libtbb and fails on several architectures | Control: severity -2 serious | | Hi Andreas | | This are actually two bugs: | - r-base dyn.load not accepting relative library names on Linux systems | and | - r-cran-rcppparallel trying to workaround the bug in dyn.load by | deducting the full path of libtbb from the architecture instead of the | correct multiarch setting and failing. | | This has nothing to do with r-cran-rstan or r-cran-rstanarm, but it | seems to be the first one to find out. I've attached patches to fix | both problems, properly re-assigned and adjusted the bugs. | | This behaviour of R dyn.load might even be considered a security | vulnerability, because loading libraries from the working directory is a | problem. | | Bastian | | -- | Kirk to Enterprise -- beam down yeoman Rand and a six-pack. | x[DELETED ATTACHMENT r-base_4.0.3-1.1.debdiff, plain text] | x[DELETED ATTACHMENT r-cran-rcppparallel_5.0.2+dfsg-3.1.debdiff, plain text]
Hi Dirk I really doubt that. Because the code as it is right now can't be used in any sensible way without an absolute path. To load anything from /usr/lib*, which is the primary use of dlopen, you need to hardcode the path. The documentation does not even mention any such specific differences to how the system loader works on non-Windows.[1] So I doubt this us used often or at all. Also this is the same problem as CVE-2016-1238, see DSA-3628[2]. I can provide a CVE id for R. Regards, Bastian [1]: https://www.rdocumentation.org/packages/base/versions/3.6.2/topics/dyn.load [2]: https://www.debian.org/security/2016/dsa-3628
Perhaps I'm misunderstanding, but there is a Debian patch for RcppParallel here: https://sources.debian.org/patches/r-cran-rcppparallel/5.0.2+dfsg-3/use_debian_packaged_libtbb.patch/ and all that does is force RcppParallel to use the Debian-provided copy of TBB as opposed to the version normally bundled and installed by RcppParallel itself. Is the implied bug in the RcppParallel patch, or in RcppParallel itself? Best, Kevin
Hi Bastian, On 10 February 2021 at 20:03, Bastian Blank wrote: | Hi Dirk | | On Wed, Feb 10, 2021 at 12:18:04PM -0600, Dirk Eddelbuettel wrote: | > As for your suggested patch to R's own dynload.c: that is very well tested | > and robust system code I do not have any real intention of changing because | > one package out of 17k at CRAN is having hickups under one (maybe suboptimal) | > Debian config. | | I really doubt that. Because the code as it is right now can't be used | in any sensible way without an absolute path. To load anything from | /usr/lib*, which is the primary use of dlopen, you need to hardcode the | path. | | The documentation does not even mention any such specific differences to | how the system loader works on non-Windows.[1] So I doubt this us used | often or at all. | | Also this is the same problem as CVE-2016-1238, see DSA-3628[2]. I | can provide a CVE id for R. Thanks for the CVE. I am happy to discuss this with R Core and one fellow in particular. Before I do so can you clarify what you think the issue is? Loading from user directories as eg ~/R/lib/mypackage/libs/mypack.so ? That is _bread and butter_ for R. Also the `.libPaths()` (for where packages are looked for, leading then for those with native code to loading of their shared libs) never has "." on. Dirk | Regards, | Bastian | | [1]: https://www.rdocumentation.org/packages/base/versions/3.6.2/topics/dyn.load | [2]: https://www.debian.org/security/2016/dsa-3628 | -- | There is a multi-legged creature crawling on your shoulder. | -- Spock, "A Taste of Armageddon", stardate 3193.9
No, the problem is loading from ".", not a specific directory. Bastian
On 10 February 2021 at 20:35, Bastian Blank wrote: | On Wed, Feb 10, 2021 at 01:30:10PM -0600, Dirk Eddelbuettel wrote: | > Thanks for the CVE. I am happy to discuss this with R Core and one fellow in | > particular. | > Before I do so can you clarify what you think the issue is? Loading from | > user directories as eg ~/R/lib/mypackage/libs/mypack.so ? | | No, the problem is loading from ".", not a specific directory. Ok, sorry, then maybe I am out of the loop ... but who/where does that? Dirk
Am Mittwoch, 10. Februar 2021, 18:55:57 CET schrieb Bastian Blank: dyn.load is used in base R to load compiled code from R packages. As far as I understand, system libraries are linked at compile time and generally do not have to be loaded explicitly. ... The documentation of dyn.load says that it accepts a path to a dynamic shared object. Do we need more than that? Johannes
Hi Johannes
Can you show some examples please? I know C and POSIX, but not much
about R.
Well, nothing forces someone to do that. Those are also shared libs and
they can be used via dlopen, what dyn.load uses.
So my change is no problem, because it only changes the behaviour for
'dyn.load("object.so")', but not for
'dyn.load("/home/foo/projects/bar/object.so")'.
The documentation does not list a search behaviour for bare library
names on non-Windows systems. So completely ignoring the system library
paths is kind of weird.
Bastian
Am Donnerstag, 11. Februar 2021, 12:20:33 CET schrieb Bastian Blank: Hello Bastian! Well, in a nutshell, loading a package in R means calling library [1] which in turn calls loadNamespace [2] which, if the package uses compiled code, calls library.dynam [3], which uses dyn.load [4] after normalizing the path to the DLL. So if you load e.g. the package MASS in R, dyn.load is called: R> debug(dyn.load) R> library(MASS) debugging in: dyn.load(file, DLLpath = DLLpath, ...) debug: .Internal(dyn.load(x, as.logical(local), as.logical(now), "")) Browse[2]> x [1] "/usr/local/lib/R/site-library/MASS/libs/MASS.so" ... I can see that it looks weird - but is it a bug? Johannes [1] https://github.com/wch/r-source/blob/trunk/src/library/base/R/ library.R#L54 [2] https://github.com/wch/r-source/blob/trunk/src/library/base/R/ namespace.R#L185 [3] https://github.com/wch/r-source/blob/trunk/src/library/base/R/ library.R#L572
On 11 February 2021 at 12:50, Johannes Ranke wrote:
| Am Donnerstag, 11. Februar 2021, 12:20:33 CET schrieb Bastian Blank:
| > Hi Johannes
|
| Hello Bastian!
|
| > On Thu, Feb 11, 2021 at 09:26:48AM +0100, Johannes Ranke wrote:
| > > dyn.load is used in base R to load compiled code from R packages.
| >
| > Can you show some examples please? I know C and POSIX, but not much
| > about R.
|
| Well, in a nutshell, loading a package in R means calling library [1] which in
| turn calls loadNamespace [2] which, if the package uses compiled code, calls
| library.dynam [3], which uses dyn.load [4] after normalizing the path to the
| DLL.
|
| So if you load e.g. the package MASS in R, dyn.load is called:
|
| R> debug(dyn.load)
| R> library(MASS)
| debugging in: dyn.load(file, DLLpath = DLLpath, ...)
| debug: .Internal(dyn.load(x, as.logical(local), as.logical(now), ""))
| Browse[2]> x
| [1] "/usr/local/lib/R/site-library/MASS/libs/MASS.so"
|
| ...
|
| > > The documentation of dyn.load says that it accepts a path to a dynamic
| > > shared object. Do we need more than that?
| >
| > So my change is no problem, because it only changes the behaviour for
| > 'dyn.load("object.so")', but not for
| > 'dyn.load("/home/foo/projects/bar/object.so")'.
| >
| > The documentation does not list a search behaviour for bare library
| > names on non-Windows systems. So completely ignoring the system library
| > paths is kind of weird.
|
| I can see that it looks weird - but is it a bug?
Exactly. It has been like that since the 1990s when R's packaging system was
set up. We have hundreds of per package shared libraries. Even the first one
I packaged for Debian (r-cran-rodbc, in 2003 if memory serves) used that.
"A feature not a bug" :)
Dirk
| Johannes
|
| [1] https://github.com/wch/r-source/blob/trunk/src/library/base/R/
| library.R#L54
|
| [2] https://github.com/wch/r-source/blob/trunk/src/library/base/R/
| namespace.R#L185
|
| [3] https://github.com/wch/r-source/blob/trunk/src/library/base/R/
| library.R#L572
Mhm, I am not sure I am seeing an argument here :) Or a missing feature, given that it was proposed to solve a problem... Johannes
On 11 February 2021 at 16:06, Johannes Ranke wrote: | > | > The documentation does not list a search behaviour for bare library | > | > names on non-Windows systems. So completely ignoring the system library | > | > paths is kind of weird. | > | | > | I can see that it looks weird - but is it a bug? | > | > Exactly. It has been like that since the 1990s | | Mhm, I am not sure I am seeing an argument here :) | | > when R's packaging system was | > set up. We have hundreds of per package shared libraries. Even the first | > one I packaged for Debian (r-cran-rodbc, in 2003 if memory serves) used | > that. | > | > "A feature not a bug" :) | | Or a missing feature, given that it was proposed to solve a problem... Or a "merely perceived by some" problem that is a actually non-problem? I have discussed prior CVEs with R Core. Poeple have over their code, the CVEs (even for Linux) mostly only covered Windows-only code in the more-or-less-eclipsed-by-RStudio IDE code (that we do not build, obviously, as it very Windows only code). Bastian knows more about security than I ever will but I still don't think there is an issue here. I'd be happy to de-escalate all this, close it, let Andreas figure what is up with RcppParallel (maybe not patching it is the best path, I don't know) and we can take up what R does internally in another venue more calmly. Dirk
Is there any consensus here. From my perception closing the issue is not
really the best solution but I'm too less involved into this.
I do not think that this is the best path since per policy we have to
avoid code copies. I wonder whether it would qualify as hackish
workaround we have no better option to choose if the solution suggested
by Bastian is no option for you.
Kind regards
Andreas.
- Create a different function in r-base that does not show that broken path behaviour. - Create a wrapper lib that uses the system loader to find the real libs. Bastian
severity 982465 wishlist thanks I am sorry but I simply see no bug in package r-base here. The RcppParallel maintainer tried a Debian-local modification to that CRAN package. That didn't work, and now fingers are pointed at r-base. Which is simply not the right way to go about this as package RcppParallel is fine at CRAN (see [1], ignore upcoming issues such the Apple M1 chip). I could and maybe should close this, but let's maybe keep it open and come back to it once RcppParallel 5.1.0 is released as some of the underlying libraries have changed or will change again under Intel oneAPI (I lost track). I will leave it to the RcppParallel to decide whether he wants working Debian package (by just packaging RcppParallel) or perfect technical purity by not including a system library yet ending up with a package that doesn't work (which was the source of this bug report). Dirk [1] https://cloud.r-project.org/web/checks/check_results_RcppParallel.html (the
Hi Dirk, Am Freitag, 19. Februar 2021, 00:27:33 CET schrieb Dirk Eddelbuettel: I think the severity of the bug [1] depends on Debian policy - is it mandatory to use system libraries if they are available? Johannes [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=982465
According to Debian Policy § 4.13[2] code copies should be avoided. The
"should" results in bugs of severity "important" against those packages
that violate this rule. It was not just for fun when I had spent my
time to follow policy. I keep on hoping to find a sensible solution
that will fit policy but I can't without your cooperation.
Kind regards
Andreas.
[2] https://www.debian.org/doc/debian-policy/ch-source.html#embedded-code-copies
But just pointing at packages for which I am upstream author / maintainer and
Debian maintainer, we already use the other approach for RcppArmadillo,
RcppEigen, RcppCCTZ, RcppDate, ... and we fudged it for BH with the empty
shell r-cran-bh which may well blow in our face BH is now at 1.75.0 which
Debian doesn't yet have.
The R manual "Writing R Extensions" even mentions it explicitly and
recognised the value of header packages. For _CRAN_ and R it is _better_ to
standardize at headers at one level _across all deployments_.
As a distro we have slightly different objectives and we cannot hit _all_
goals _all the time_. What we have right now is suboptimal for Debian ("in
theory we could do better") but practically speaking I quite strongly feel
that we do is the right way to do it.
(And I am quite frankly more mad, or I should say, "disappointed" at the
_CRAN_ level where e.g. the CCTZ headers are now in (last I looked) three
more packages (including lubridate, famously), the new package clock does the
same to the Date headers in RcppDate and on and on. Some people are very keen
to reinvent and slap "their" logo on things, and there is no stopping them.)
All that said, I am course all for experimentation and trying to make it
better. But what we found here is that the current attempt at trying it for
RcppParallel did not exactly work out fully. So if it were me, I would
backtrack and package "as is" because to me the primary directive is to get
current and working software to our users. But we are Debian and maintainers
can do as they please. Which in my case means saying no to a bug report
suggesting I alter R itself for a Debian-local-and-only hack. As I said, if
it were of general help and use (and I may misjudge it, convince me) I will
_of course_ help and carry it to R Core for vetting and inclusion
upstream. But as far as I can tell that is not the situation here. So please
leave the r-base package out of all micro-fights you are in for RcppParallel.
Dirk