search

TritonDataCenter / node-verror

Rich JavaScript errors
MIT License
1.18k stars 61 forks source link

Do not call sprintf with one argument #63

Open hekike opened 5 years ago

hekike commented 5 years ago

PR https://cr.joyent.us/#/c/5299

Safe user inputs in messages

Addresses the issue when someone builds message manually and it contains user-input with formatting characters instead of using sprintf.

For example: In this case VError throws without this change:

var myInput = "encoded%stext"
new VError(myInput + " invalid input")

Related to https://github.com/joyent/node-verror/issues/57

Performance Improvement

Calling sprintf is expensive, do not call when zero arguments are passed. In some scenarios, an application needs to create hundreds of VError(s).

To test the change's effect:

for(var i = 0; i < 10000; i++) {
        new mod_verror.VError('Foo')
}

On my 2017 MBP 3.5Ghz, I see a 27% performance improvement after the change.

Before: screen shot 2018-12-31 at 10 29 47 am

davepacheco commented 5 years ago

Thanks for submitting this!

I really don't like the change in behavior regarding format specifiers like '%s' in a single-argument string. Similar ideas have been discussed extensively in #57, #8, and a few others. In particular, this question and the reply under it. I get that this is a rough edge, and I'm sorry for the pain it causes, but I find the alternative painful, too (inconsistent behavior regarding cases that are otherwise the same from a user's perspective).

57 proposes an alternate constructor that would avoid this particular rough edge for people who want that. I think that's the way to go here.

davepacheco commented 5 years ago

As an example, I think it's pretty confusing that VError('%s') would produces a valid Error with message '%s' while VError('%s %s', 'one') would blow up.

hekike commented 5 years ago

@davepacheco thanks for the feedback, I picked up the work from https://github.com/joyent/node-verror/issues/57 with adding the modifications you asked in the CL: https://cr.joyent.us/#/c/4394

New CL: https://cr.joyent.us/#/c/5360/

Please review.

gabegorelick commented 5 years ago

Will this be done soon?

davepacheco commented 5 years ago

@gabegorelick I did provide feedback on the change back in January but I believe the author went a different route. I just tried to pick it up myself and apply the feedback but there's a little more work left than I realized. The new change is here:

https://cr.joyent.us/#/c/6608/

The main difference from review 5360 is that I've refactored the code as I suggested in 5360. Unfortunately, the test suite doesn't pass. I'm not sure if it wasn't run before or if my changes made it worse, but it looks to me like more work is needed to separate out the tests that can be run with PError from those that can't (because they test format strings). Since I haven't fixed that, I don't know if there are other regressions lurking.

I think this is the best starting point if somebody wants to pick up the test suite work. I'm not sure when I'll get to that again.

trentm commented 4 years ago

My history of verror work that led to the skipPrintf option to VError and the PError class. (@davepacheco has an earlier history here.)

My current intent: