Avoid `fmt` when constructor already does it #3720

Ericson2314 · 2020-06-18T17:59:23Z

We have a larger problem that passing computed strings to the first
variable argument of many exception constructors is unsafe because that
first variable argument is interpreted not as a plain string, but format
string, and if it contains '%' boost::format will abort, since there are
no arguments to the format string.

In this particular instance '%' was used as part of an escape code in a
URL, which, when the download failed, caused Nix to abort displaying the
FileTransferError.

#3724 will fix the underlying issue. This is just a cleanup for brevity's sake.

Most of the changes are very straight forward, but I did refactor a bit more aggressively with the SQLite errors, taking SysError my guide.

CC @bburdette

matthewbauer · 2020-06-18T18:19:19Z

There are a few similar cases in:

nix/src/libutil/util.cc

Line 1020 in 6c000ee

throw ExecError(res.first, fmt("program '%1%' %2%", program, statusToString(res.first)));
nix/src/libutil/util.cc

Line 1147 in 6c000ee

throw ExecError(status, fmt("program '%1%' %2%", options.program, statusToString(status)));
nix/src/libfetchers/mercurial.cc

Line 200 in 6c000ee

throw ExecError(e.status, fmt("'hg pull' %s", statusToString(e.status)));
nix/src/libmain/shared.cc

Line 52 in 6c000ee

printMsg(lvl, fmt("this derivation will be built:"));
nix/src/libstore/sqlite.cc

Line 21 in 6c000ee

? fmt("SQLite database '%s' is busy (SQLITE_PROTOCOL)", path)

(most of those won't have a percent though, but best to be safe)

edolstra · 2020-06-19T09:29:11Z

This is a regression compared to 2.3, where both throw Error("foo %s") and throw Error(fmt("foo %s")) printed error: foo %s. This is because we had a specialisation for fmt:

/* A helper for formatting strings. ‘fmt(format, a_0, ..., a_n)’ is
   equivalent to ‘boost::format(format) % a_0 % ... %
   ... a_n’. However, ‘fmt(s)’ is equivalent to ‘s’ (so no %-expansion
   takes place). */

inline std::string fmt(const std::string & s)
{
    return s;
}

...

We should restore that specialisation.

Ericson2314 · 2020-06-21T16:15:34Z

#3724 is better to fix the underlying issue, so I'll make this just a duplicate formatting cleanup.

Ericson2314 · 2020-06-21T19:15:48Z

Guess clang has different semantics around protected here.

Ericson2314 · 2020-06-22T03:08:32Z

Huh, this is the second PR of mine where there are weird perl issues.

edolstra · 2020-06-22T08:28:04Z

What do the SQLite changes do?

Ericson2314 · 2020-06-22T14:36:35Z

@edolstra they shouldn't have any observable effect.

The idea is the helper function takes var args like the error constructors, formatiting the users format string. Also, the errors themselves store the original information (error number and extended error number) should somebody want to display the information differently.

With @bburdette's approval, I would complete the process of separating structure and presentation by making SysError and thse SQLite errors just store the user's original hintformat value, and then render the boilerplate stuff (with the strerr and similiar for the error numbers, and shortened message for the SQLite is busy) on demand when the exception is caught and rendered near main.

stale · 2021-02-12T05:13:04Z

I marked this as stale due to inactivity. → More info

stale · 2022-04-16T09:26:36Z

I closed this issue due to inactivity. → More info

Ericson2314 · 2022-04-19T01:37:12Z

I think this is till a good cleanup. Rebased it.

There is a correctnes issue here, but NixOS#3724 will fix that. This is just a cleanup for brevity's sake.

thufschmitt

I think this is till a good cleanup

Indeed, thanks for the rebase :)

There’s just an issue with the sqlite stuff where you introduce a nasty little footgun. Other than that 👍

thufschmitt · 2022-04-20T09:42:41Z

src/libstore/sqlite.hh

+    int errNo, extendedErrNo;
+
+    template<typename... Args>
+    [[noreturn]] static void throw_(sqlite3 * db, const std::string & fs, const Args & ... args);


Unless I’m missing something, this is very fragile since the definition of the templated function isn’t in the header − so only the instances of the template that happen to be used in sqlite.cc will be available outside (which happens to be the case because the only usages outside of sqlite.cc seem to be whith args being a single string)

Oh good catch!

Fixed this.

Fixed it an even better way!

This ensures that use-sites properly trigger new monomorphisations on one hand, and on the other hand keeps the main `sqlite.hh` clean and interface-only. I think that is good practice in general, but in this situation in particular we do indeed have `sqlite.hh` users that don't need the `throw_` function.

The templating is very superficial

thufschmitt

Looks good now :)

Set to merge once the CI is green ✔️

orbekk · 2022-04-21T01:20:57Z

I wonder if this may have broken the build (run link:

error: builder for '/nix/store/diz7s7lkp2an9aa8vrwb225mbghgksfn-nix-2.9.0pre20220420_9345b4e.drv' failed with exit code 2;
       last 10 log lines:
       >   CXX    src/libstore/remote-store.o
       > src/libstore/sqlite.cc:28:20: error: calling a protected constructor of class 'nix::SQLiteBusy'
       >         auto exp = SQLiteBusy(path, err, exterr, std::move(hf));
       >                    ^
       > src/libstore/sqlite.hh:122:23: note: declared protected here
       > MakeError(SQLiteBusy, SQLiteError);
       >                       ^
       > 1 error generated.
       > make: *** [mk/patterns.mk:3: src/libstore/sqlite.o] Error 1
       > make: *** Waiting for unfinished jobs....
       For full logs, run 'nix log /nix/store/diz7s7lkp2an9aa8vrwb225mbghgksfn-nix-2.9.0pre20220420_9345b4e.drv'.

thufschmitt · 2022-04-21T07:31:42Z

I wonder if this may have broken the build

It did indeed :/ Looking at it right now

Ericson2314 · 2022-04-21T14:06:31Z

Thanks for fixing!

Ericson2314 mentioned this pull request Jun 18, 2020

Add response body to network errors #3714

Merged

Ericson2314 changed the title ~~Prevent '%' in URL from causing crashes~~ WIP: Restore specialization that makes single string arg not a format string Jun 19, 2020

bburdette mentioned this pull request Jun 19, 2020

Hintfmt percent test, and fix #3724

Merged

Ericson2314 changed the title ~~WIP: Restore specialization that makes single string arg not a format string~~ WIP: Use constructor rather than adittional fmt for tidyness Jun 21, 2020

Ericson2314 changed the title ~~WIP: Use constructor rather than adittional fmt for tidyness~~ WIP: Use constructor rather than additional fmt for tidiness Jun 21, 2020

Ericson2314 force-pushed the fix-url-format branch from 639e20d to de1ec0f Compare June 21, 2020 18:52

Ericson2314 changed the title ~~WIP: Use constructor rather than additional fmt for tidiness~~ Avoid fmt when constructor already does it Jun 21, 2020

Ericson2314 changed the title ~~Avoid fmt when constructor already does it~~ WIP: Avoid fmt when constructor already does it Jun 21, 2020

stale bot added the stale label Feb 12, 2021

stale bot closed this Apr 16, 2022

Ericson2314 reopened this Apr 19, 2022

stale bot removed the stale label Apr 19, 2022

Ericson2314 force-pushed the fix-url-format branch from de1ec0f to 0081b75 Compare April 19, 2022 01:32

Avoid fmt when constructor already does it

75b62e5

There is a correctnes issue here, but NixOS#3724 will fix that. This is just a cleanup for brevity's sake.

Ericson2314 force-pushed the fix-url-format branch from 0081b75 to 75b62e5 Compare April 19, 2022 01:45

Ericson2314 changed the title ~~WIP: Avoid fmt when constructor already does it~~ Avoid fmt when constructor already does it Apr 19, 2022

thufschmitt requested changes Apr 20, 2022

View reviewed changes

Ericson2314 added 3 commits April 20, 2022 16:53

Merge remote-tracking branch 'upstream/master' into fix-url-format

3c22044

Actually, solve this in a lighter-weight way

f63b0f4

The templating is very superficial

thufschmitt approved these changes Apr 20, 2022

View reviewed changes

thufschmitt enabled auto-merge April 20, 2022 17:56

thufschmitt merged commit 9345b4e into NixOS:master Apr 20, 2022

Ericson2314 deleted the fix-url-format branch April 20, 2022 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid `fmt` when constructor already does it #3720

Avoid `fmt` when constructor already does it #3720

Ericson2314 commented Jun 18, 2020 •

edited

matthewbauer commented Jun 18, 2020 •

edited

edolstra commented Jun 19, 2020

Ericson2314 commented Jun 21, 2020

Ericson2314 commented Jun 21, 2020

Ericson2314 commented Jun 22, 2020

edolstra commented Jun 22, 2020

Ericson2314 commented Jun 22, 2020

stale bot commented Feb 12, 2021

stale bot commented Apr 16, 2022

Ericson2314 commented Apr 19, 2022

thufschmitt left a comment

thufschmitt Apr 20, 2022

Ericson2314 Apr 20, 2022

Ericson2314 Apr 20, 2022

Ericson2314 Apr 20, 2022

thufschmitt left a comment

orbekk commented Apr 21, 2022

thufschmitt commented Apr 21, 2022

Ericson2314 commented Apr 21, 2022

Avoid fmt when constructor already does it #3720

Avoid fmt when constructor already does it #3720

Conversation

Ericson2314 commented Jun 18, 2020 • edited

matthewbauer commented Jun 18, 2020 • edited

edolstra commented Jun 19, 2020

Ericson2314 commented Jun 21, 2020

Ericson2314 commented Jun 21, 2020

Ericson2314 commented Jun 22, 2020

edolstra commented Jun 22, 2020

Ericson2314 commented Jun 22, 2020

stale bot commented Feb 12, 2021

stale bot commented Apr 16, 2022

Ericson2314 commented Apr 19, 2022

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt Apr 20, 2022

Choose a reason for hiding this comment

Ericson2314 Apr 20, 2022

Choose a reason for hiding this comment

Ericson2314 Apr 20, 2022

Choose a reason for hiding this comment

Ericson2314 Apr 20, 2022

Choose a reason for hiding this comment

thufschmitt left a comment

Choose a reason for hiding this comment

orbekk commented Apr 21, 2022

thufschmitt commented Apr 21, 2022

Ericson2314 commented Apr 21, 2022

Avoid `fmt` when constructor already does it #3720

Avoid `fmt` when constructor already does it #3720

Ericson2314 commented Jun 18, 2020 •

edited

matthewbauer commented Jun 18, 2020 •

edited