Secure alloc compile time #426
Conversation
Pull from osp
Merge jsoncpp latest changes to forked master
Merge jsoncpp latest changes to forked master
Before freeing data use memset in Value to 0 Use secure allocator globally when configured but leave as much backward compatible as possible in the public interfaces (particularly asString and Reader).
| @@ -230,7 +230,7 @@ class JSON_API Value { | |||
|
|
|||
| struct StringStorage { | |||
| unsigned policy_: 2; | |||
| unsigned length_: 30; // 1GB max | |||
| size_t length_: 30; // 1GB max | |||
There was a problem hiding this comment.
This could break binary compatibility. Was this necessary for compilation?
There was a problem hiding this comment.
Hi chris, sorry i changed it to make life easier but for backward compatibility you're right, i've fixed this
|
Whew! Tons of changes, but way less than in that previous monster PR. |
| @@ -572,6 +573,7 @@ Json::Value obj_value(Json::objectValue); // {} | |||
| void setComment(const char* text, size_t len); | |||
|
|
|||
| char* comment_; | |||
| size_t len_; | |||
There was a problem hiding this comment.
I need to add another #ifdef around this
* Remove added instance variables when not secure * Change to original types * getCStringLength available in secure mode only * asString now returns the secure version if compiled in secure mode
| // If non-zero, the library zeroes any memory that it has allocated before | ||
| // it frees its | ||
| #ifndef JSON_USE_SECURE_MEMORY | ||
| #define JSON_USE_SECURE_MEMORY 1 |
There was a problem hiding this comment.
0 by default -- at least until we measure the runtime/memory cost of using this allocator.... Oh, actually, since there are binary-incompatible changes, this will have to be 0 by default until version 2.0.0, and forever if the cost is significant.
There was a problem hiding this comment.
We'll have to check whether the typedefs alter the symbols. If so, then we'll need to switch those all to macros (poor-man's template). Again, that's to ensure binary-compatibility. It's painful, but our unusual care for binary-compatibility is one reason why a distro like Debain includes JsonCpp.
|
In our defense, I'd like to state that none of us wrote this library. Baptiste, the primary author, had good reasons for everything at the time. We're now maintaining it and hopefully letting it evolve in a positive direction. So please excuse the oddities in implementation. People were very skeptical when I added the string-prefixing, but it has proven invaluable. I should have more time this weekend or next. I plan to accept the typedefs (or macros, if necessary). Then you can rebase with a much smaller pull-request. |
Remove all the lengths on strings so that we use the memory already allocated
It wasn't taking into account the length prefix
|
Thanks so much for all of this Chris, it's really very much appreciated all the effort you've put in on this. Christopher |
|
Ok. This is looking pretty good. But I'm swamped this weekend. I'll rebase as soon as I can... |
|
I just wanted to re-iterate my thanks for all your support you've put in an amazing amount of time on this to help me get this through and it's thoroughly appreciated! |
|
Would you rebase to the |
Hi Chris, I hope all's well, here's a version of the secure allocator using typedefs in config similar to the way it's done with UInt; i've checked it in with 1 as secure allocator at the moment as i'd like to see what the regression test thought of it first of all. What would you prefer the default to be though?
Thanks again for your support on this,
Christopher