Skip to content

Commit 798aff1

Browse files
committed
Fix issue when using intermediate string returns
In MSVC the following can lead to segfault: sass_copy_c_string(stream.str().c_str()); Reason is that the string returned by str() is disposed before sass_copy_c_string is invoked. The string is actually a stack object, so indeed nobody is holding on to it. So it seems perfectly fair to release it right away. So the const char* by c_str will point to invalid memory. I'm not sure if this is the behavior for all compiler, but I'm pretty sure we would have gotten more issues reported if that would be the case. Wrapping it in a functions seems the cleanest approach as the function must hold on to the stack variable until it's done.
1 parent 8160ba9 commit 798aff1

File tree

4 files changed

+54
-14
lines changed

4 files changed

+54
-14
lines changed

src/sass.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,24 @@ extern "C" {
7070
}
7171

7272
}
73+
74+
namespace Sass {
75+
76+
// helper to aid dreaded MSVC debug mode
77+
char* sass_copy_string(std::string str)
78+
{
79+
// In MSVC the following can lead to segfault:
80+
// sass_copy_c_string(stream.str().c_str());
81+
// Reason is that the string returned by str() is disposed before
82+
// sass_copy_c_string is invoked. The string is actually a stack
83+
// object, so indeed nobody is holding on to it. So it seems
84+
// perfectly fair to release it right away. So the const char*
85+
// by c_str will point to invalid memory. I'm not sure if this is
86+
// the behavior for all compiler, but I'm pretty sure we would
87+
// have gotten more issues reported if that would be the case.
88+
// Wrapping it in a functions seems the cleanest approach as the
89+
// function must hold on to the stack variable until it's done.
90+
return sass_copy_c_string(str.c_str());
91+
}
92+
93+
}

src/sass.hpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@
4545
// include C-API header
4646
#include "sass/base.h"
4747

48+
// For C++ helper
49+
#include <string>
50+
4851
// output behaviours
4952
namespace Sass {
5053

@@ -57,7 +60,11 @@ namespace Sass {
5760
const static Sass_Output_Style INSPECT = SASS_STYLE_INSPECT;
5861
const static Sass_Output_Style TO_SASS = SASS_STYLE_TO_SASS;
5962

60-
};
63+
// helper to aid dreaded MSVC debug mode
64+
// see implementation for more details
65+
char* sass_copy_string(std::string str);
66+
67+
}
6168

6269
// input behaviours
6370
enum Sass_Input_Style {

src/sass_context.cpp

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,17 @@
1717

1818
#define LFEED "\n"
1919

20+
// C++ helper
21+
namespace Sass {
22+
// see sass_copy_c_string(std::string str)
23+
static inline JsonNode* json_mkstream(const std::stringstream& stream)
24+
{
25+
// hold on to string on stack!
26+
std::string str(stream.str());
27+
return json_mkstring(str.c_str());
28+
}
29+
}
30+
2031
extern "C" {
2132
using namespace Sass;
2233

@@ -103,10 +114,9 @@ extern "C" {
103114
json_append_member(json_err, "line", json_mknumber((double)(e.pstate.line+1)));
104115
json_append_member(json_err, "column", json_mknumber((double)(e.pstate.column+1)));
105116
json_append_member(json_err, "message", json_mkstring(e.what()));
106-
json_append_member(json_err, "formatted", json_mkstring(msg_stream.str().c_str()));
107-
117+
json_append_member(json_err, "formatted", json_mkstream(msg_stream));
108118
try { c_ctx->error_json = json_stringify(json_err, " "); } catch(...) {}
109-
c_ctx->error_message = sass_copy_c_string(msg_stream.str().c_str());
119+
c_ctx->error_message = sass_copy_string(msg_stream.str());
110120
c_ctx->error_text = sass_copy_c_string(e.what());
111121
c_ctx->error_status = 1;
112122
c_ctx->error_file = sass_copy_c_string(e.pstate.path);
@@ -123,9 +133,9 @@ extern "C" {
123133
msg_stream << "Unable to allocate memory: " << ba.what() << std::endl;
124134
json_append_member(json_err, "status", json_mknumber(2));
125135
json_append_member(json_err, "message", json_mkstring(ba.what()));
126-
json_append_member(json_err, "formatted", json_mkstring(msg_stream.str().c_str()));
136+
json_append_member(json_err, "formatted", json_mkstream(msg_stream));
127137
try { c_ctx->error_json = json_stringify(json_err, " "); } catch(...) {}
128-
c_ctx->error_message = sass_copy_c_string(msg_stream.str().c_str());
138+
c_ctx->error_message = sass_copy_string(msg_stream.str());
129139
c_ctx->error_text = sass_copy_c_string(ba.what());
130140
c_ctx->error_status = 2;
131141
c_ctx->output_string = 0;
@@ -138,9 +148,9 @@ extern "C" {
138148
msg_stream << "Internal Error: " << e.what() << std::endl;
139149
json_append_member(json_err, "status", json_mknumber(3));
140150
json_append_member(json_err, "message", json_mkstring(e.what()));
141-
json_append_member(json_err, "formatted", json_mkstring(msg_stream.str().c_str()));
151+
json_append_member(json_err, "formatted", json_mkstream(msg_stream));
142152
try { c_ctx->error_json = json_stringify(json_err, " "); } catch(...) {}
143-
c_ctx->error_message = sass_copy_c_string(msg_stream.str().c_str());
153+
c_ctx->error_message = sass_copy_string(msg_stream.str());
144154
c_ctx->error_text = sass_copy_c_string(e.what());
145155
c_ctx->error_status = 3;
146156
c_ctx->output_string = 0;
@@ -153,9 +163,9 @@ extern "C" {
153163
msg_stream << "Internal Error: " << e << std::endl;
154164
json_append_member(json_err, "status", json_mknumber(4));
155165
json_append_member(json_err, "message", json_mkstring(e.c_str()));
156-
json_append_member(json_err, "formatted", json_mkstring(msg_stream.str().c_str()));
166+
json_append_member(json_err, "formatted", json_mkstream(msg_stream));
157167
try { c_ctx->error_json = json_stringify(json_err, " "); } catch(...) {}
158-
c_ctx->error_message = sass_copy_c_string(msg_stream.str().c_str());
168+
c_ctx->error_message = sass_copy_string(msg_stream.str());
159169
c_ctx->error_text = sass_copy_c_string(e.c_str());
160170
c_ctx->error_status = 4;
161171
c_ctx->output_string = 0;
@@ -168,9 +178,9 @@ extern "C" {
168178
msg_stream << "Internal Error: " << e << std::endl;
169179
json_append_member(json_err, "status", json_mknumber(4));
170180
json_append_member(json_err, "message", json_mkstring(e));
171-
json_append_member(json_err, "formatted", json_mkstring(msg_stream.str().c_str()));
181+
json_append_member(json_err, "formatted", json_mkstream(msg_stream));
172182
try { c_ctx->error_json = json_stringify(json_err, " "); } catch(...) {}
173-
c_ctx->error_message = sass_copy_c_string(msg_stream.str().c_str());
183+
c_ctx->error_message = sass_copy_string(msg_stream.str());
174184
c_ctx->error_text = sass_copy_c_string(e);
175185
c_ctx->error_status = 4;
176186
c_ctx->output_string = 0;
@@ -184,7 +194,7 @@ extern "C" {
184194
json_append_member(json_err, "status", json_mknumber(5));
185195
json_append_member(json_err, "message", json_mkstring("unknown"));
186196
try { c_ctx->error_json = json_stringify(json_err, " "); } catch(...) {}
187-
c_ctx->error_message = sass_copy_c_string(msg_stream.str().c_str());
197+
c_ctx->error_message = sass_copy_string(msg_stream.str());
188198
c_ctx->error_text = sass_copy_c_string("unknown");
189199
c_ctx->error_status = 5;
190200
c_ctx->output_string = 0;

src/units.hpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,9 @@ namespace Sass {
7777
ss << "Incompatible units: ";
7878
ss << "'" << unit_to_string(a) << "' and ";
7979
ss << "'" << unit_to_string(b) << "'";
80-
msg = ss.str().c_str();
80+
// hold on to string on stack!
81+
std::string str(ss.str());
82+
msg = str.c_str();
8183
}
8284
virtual const char* what() const throw()
8385
{

0 commit comments

Comments
 (0)