Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A fresh -Wformat-security issue under r-devel #1287

Closed
eddelbuettel opened this issue Nov 26, 2023 · 9 comments
Closed

A fresh -Wformat-security issue under r-devel #1287

eddelbuettel opened this issue Nov 26, 2023 · 9 comments
Assignees

Comments

@eddelbuettel
Copy link
Member

eddelbuettel commented Nov 26, 2023

Update 2023-11-28: If you came here because of a similar message in your package please read on and see particularly this comment below for the fairly simple fix.


While working on an update for RQuantLib with a few r-devel discovered minor changes, CRAN and I both came across a new nag this time from -Wformat-security.. Our glue code in src/attributes.cpp does

                     << "    if (rcpp_isError_gen) {" << std::endl
                     << "        SEXP rcpp_msgSEXP_gen = Rf_asChar(rcpp_result_gen);" << std::endl
                     << "        UNPROTECT(1);" << std::endl
                     << "        Rf_error(CHAR(rcpp_msgSEXP_gen));" << std::endl
                     << "    }" << std::endl

and the Rf_error(someCharvariablehere) now makes the compiler bark under -Wformat-security:

RcppExports.cpp:180:18: warning: format string is not a string literal (potentially insecure) [-Wformat-security]

The fix is pretty easy: add a "%s". I will take care of that shortly.

@DISOhda
Copy link

DISOhda commented Nov 28, 2023

Hello,

Today I was contacted by CRAN to take care of -Wformat-security compiler warnings which are identical to yours, otherwise my package (https://github.com/DISOhda/PoissonBinomial) would be removed at 2023-12-12.

In the check log (https://www.r-project.org/nosvn/R.check/r-devel-linux-x86_64-debian-clang/PoissonBinomial-00check.html), there were 27 identical warnings like

RcppExports.cpp:45:18: warning: format string is not a string literal (potentially insecure) [-Wformat-security]

All the indicated lines are identical:

Rf_error(CHAR(rcpp_msgSEXP_gen));

When looking at the install log (https://www.r-project.org/nosvn/R.check/r-devel-linux-x86_64-debian-clang/PoissonBinomial-00install.html), it becomes clear that the security warnings originate from Rcpp header print.h:

/home/hornik/tmp/R.check/r-devel-clang/Work/build/Packages/Rcpp/include/Rcpp/print.h:30:26: warning: format string is not a string literal (potentially insecure) [-Wformat-security]
   30 |     Rf_warningcall(call, s.c_str());
      |                          ^~~~~~~~~
/home/hornik/tmp/R.check/r-devel-clang/Work/build/Packages/Rcpp/include/Rcpp/print.h:30:26: note: treat the string as an argument to avoid this
   30 |     Rf_warningcall(call, s.c_str());
      |                          ^
      |                          "%s", 

Seems to be the very issue that you pointed out. Maybe there are other packages that are affected by this, too.

Is there something I have to do or do I just have to wait for you to release a fixed package version and the warning to vanish?

Best

@eddelbuettel
Copy link
Member Author

eddelbuettel commented Nov 28, 2023

Yes, I actually got five such emails myself today for packages of mine using Rcpp.

The fix is simple thanks to PR #1288 we made two days ago. Install Rcpp 1.0.11.5 from the Rcpp drat repo via, e.g.,

 Rscript -e 'install.packages("Rcpp", repos=c("https://RcppCore.github.io/drat", getOption("repos")))'

and then re-run compileAttributes(). That will fix the RcppExports.cpp file for you. You then need to upload your updated package to CRAN. It has no change in its run-time dependency on Rcpp so you do not need to change anything in DESCRIPTION related to Rcpp (but a new upload of course needs a new version). Just re-run compileAttributes(), and increment your version (and do whatever else R(-devel) CMD check --as-cran may need).

@TobiasFellinger
Copy link

Thanks for the quick fix and the comment on how to fix issues with packages linking to Rcpp.

@eddelbuettel
Copy link
Member Author

My pleasure! As you see in this ticket, I actually hit is myself updating a package a few days ago. r-devel and a new g++ are good at this.

And I am sure we will hear more about it here, at StackOverflow, or on lists such as rcpp-devel or r-package-devel so when you see it by all means feel free to share the word 😀

@DISOhda
Copy link

DISOhda commented Nov 28, 2023

You then need to upload your updated package to CRAN. It has no change in its run-time dependency on Rcpp so you do not need to change anything in DESCRIPTION. Just re-run compileAttributes().

I tried to resubmit my fixed package without changing DESCRIPTION. But it was rejected because it is unchanged, i.e. I got warned that the date is quite old and that the version string is the same as the existing version's. So, I had to increase it and change the date to get it approved.

But still, thank you very much for your quick response. It is very much appreciated.

@eddelbuettel
Copy link
Member Author

eddelbuettel commented Nov 28, 2023

So, I had to increase it and change the date to get it approved.

That is expected.

You actually changed code, so that requires a version that is strictly monotonically higher than the one it replaces.

PS I see where I confused you writing "so you do not need to change anything in DESCRIPTION". Will edit.

@DISOhda
Copy link

DISOhda commented Nov 29, 2023

PS I see where I confused you writing "so you do not need to change anything in DESCRIPTION". Will edit.

Purely my mistake. I should have known not to take that so literally. Anyway...

While I am at it: Which Rcpp version should be specified as the minimum requirement in DESCRIPTION?

fj86 added a commit to fj86/PoissonBinomial that referenced this issue Nov 29, 2023
@eddelbuettel
Copy link
Member Author

eddelbuettel commented Nov 29, 2023

While I am at it: Which Rcpp version should be specified as the minimum requirement in DESCRIPTION?

That is what my comment is about: No change needed in DESCRIPTION as that determines a run-time dependency but we have no change in run-time here. Remember that these warnings come from a fixed, static, generated file RcppExports.cpp that does not change no matter what version the user of your package has. Once Rcpp 1.0.12 is at CRAN come January you depend on that. Depending on the development version you used will create trouble as that version is not yet at CRAN.

Makes sense?

eddelbuettel added a commit to eddelbuettel/extraDistr that referenced this issue Nov 29, 2023
edzer added a commit to r-spatial/sf that referenced this issue Nov 29, 2023
paulnorthrop added a commit to paulnorthrop/revdbayes that referenced this issue Dec 1, 2023
zhuxr11 added a commit to zhuxr11/markovmix that referenced this issue Dec 2, 2023
paulnorthrop added a commit to paulnorthrop/itp that referenced this issue Dec 2, 2023
paulnorthrop added a commit to paulnorthrop/exdex that referenced this issue Dec 2, 2023
paulnorthrop added a commit to paulnorthrop/rust that referenced this issue Dec 2, 2023
loelschlaeger added a commit to loelschlaeger/oeli that referenced this issue Dec 8, 2023
@eddelbuettel
Copy link
Member Author

Rcpp 1.0.12 arrived at CRAN on Jan 9 as planned so this can be closed.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants