@@ -9,9 +9,9 @@ not-always-obvious ways, or impose a pattern that tends to cause hard to find
9
9
bugs, or bugs that appear later. Every one has been submitted to code review
10
10
multiple times.
11
11
12
- ### 1. Directly dereferencing a pointer without checking for validity first
12
+ ## 1. Directly dereferencing a pointer without checking for validity first
13
13
14
- ``` C++
14
+ ``` cpp
15
15
int myBadMethod (const nlohmann::json& j){
16
16
const int* myPtr = j.get_if<int >();
17
17
return * myPtr;
@@ -20,34 +20,34 @@ int myBadMethod(const nlohmann::json& j){
20
20
21
21
This pointer is not guaranteed to be filled, and could be a null dereference.
22
22
23
- ### 2. String views aren't null terminated
23
+ ## 2. String views aren't null terminated
24
24
25
- ```C++
25
+ ```cpp
26
26
int getIntFromString(const std::string_view s){
27
27
return std::atoi(s.data());
28
28
}
29
29
```
30
30
31
31
This will give the right answer much of the time, but has the possibility to
32
- fail when string_view is not null terminated. Use from_chars instead, which
32
+ fail when ` string_view ` is not null terminated. Use ` from_chars ` instead, which
33
33
takes both a pointer and a length
34
34
35
- ### 3. Not handling input errors
35
+ ## 3. Not handling input errors
36
36
37
- ``` C++
37
+ ``` cpp
38
38
int getIntFromString (const std::string& s){
39
39
return std::atoi(s.c_str());
40
40
}
41
41
```
42
42
43
43
In the case where the string is not representable as an int, this will trigger
44
44
undefined behavior at system level. Code needs to check for validity of the
45
- string, ideally with something like from_chars, and return the appropriate error
46
- code.
45
+ string, ideally with something like ` from_chars` , and return the appropriate
46
+ error code.
47
47
48
- ### 4. Walking off the end of a string
48
+ ## 4. Walking off the end of a string
49
49
50
- ```C++
50
+ ```cpp
51
51
std::string getFilenameFromPath(const std::string& path){
52
52
size_t index = path.find("/");
53
53
if (index != std::string::npos){
@@ -58,9 +58,9 @@ std::string getFilenameFromPath(const std::string& path){
58
58
}
59
59
```
60
60
61
- ### 5. Using methods that throw (or not handling bad inputs)
61
+ ## 5. Using methods that throw (or not handling bad inputs)
62
62
63
- ``` C++
63
+ ``` cpp
64
64
int myBadMethod (nlohmann::json& j){
65
65
return j.get<int >();
66
66
}
@@ -90,7 +90,7 @@ Commonly used methods that fall into this pattern:
90
90
- std::stol
91
91
- std::stoll
92
92
93
- #### Special note: JSON
93
+ ### Special note: JSON
94
94
95
95
`nlohmann::json::parse` by default
96
96
[throws](https://json.nlohmann.me/api/basic_json/parse/) on failure, but also
@@ -103,7 +103,7 @@ accepts an optional argument that causes it to not throw: set the 4th argument
103
103
to `replace`. Although `ignore` preserves content 1:1, `replace` is preferred
104
104
from a security point of view.
105
105
106
- #### Special note: Boost
106
+ ### Special note: Boost
107
107
108
108
there is a whole class of boost asio functions that provide both a method that
109
109
throws on failure, and a method that accepts and returns an error code. This is
@@ -116,7 +116,7 @@ asio methods, and prefer the one that returns an error code instead of throwing.
116
116
- boost::asio::ip::tcp::acceptor::listen();
117
117
- boost::asio::ip::address::make_address();
118
118
119
- ### 6. Blocking functions
119
+ ## 6. Blocking functions
120
120
121
121
bmcweb uses a single reactor for all operations. Blocking that reactor for any
122
122
amount of time causes all other operations to stop. The common blocking
@@ -138,15 +138,15 @@ system, the fact that most filesystem accesses are into tmpfs (and therefore
138
138
should be "fast" most of the time) and in general how little the filesystem is
139
139
used in practice.
140
140
141
- ### 7. Lack of locking between subsequent calls
141
+ ## 7. Lack of locking between subsequent calls
142
142
143
143
While global data structures are discouraged, they are sometimes required to
144
144
store temporary state for operations that require it. Given the single threaded
145
145
nature of bmcweb, they are not required to be explicitly threadsafe, but they
146
146
must be always left in a valid state, and checked for other uses before
147
147
occupying.
148
148
149
- ```C++
149
+ ```cpp
150
150
std::optional<std::string> currentOperation;
151
151
void firstCallbackInFlow(){
152
152
currentOperation = "Foo";
@@ -159,9 +159,9 @@ void secondCallbackInFlow(){
159
159
In the above case, the first callback needs a check to ensure that
160
160
currentOperation is not already being used.
161
161
162
- ### 8. Wildcard reference captures in lambdas
162
+ ## 8. Wildcard reference captures in lambdas
163
163
164
- ```
164
+ ``` cpp
165
165
std::string x; auto mylambda = [&](){
166
166
x = "foo";
167
167
}
@@ -173,11 +173,12 @@ async bmcweb code. While capturing by reference can be useful, given how
173
173
difficult these types of bugs are to triage, bmcweb explicitly requires that all
174
174
code captures variables by name explicitly, and calls out each variable being
175
175
captured by value or by reference. The above prototypes would change to
176
- [ &x] ( ) ... Which makes clear that x is captured, and its lifetime needs tracked.
176
+ `[&x]()...` Which makes clear that x is captured, and its lifetime needs
177
+ tracked.
177
178
178
- ### 9. URLs should end in "/"
179
+ ## 9. URLs should end in "/"
179
180
180
- ``` C++
181
+ ```cpp
181
182
BMCWEB("/foo/bar");
182
183
```
183
184
@@ -188,9 +189,9 @@ the route ending in slash and the one without. This allows both URLs to be used
188
189
by users. While many specifications do not require this, it resolves a whole
189
190
class of bug that we've seen in the past.
190
191
191
- ### 10. URLs constructed in aggregate
192
+ ## 10. URLs constructed in aggregate
192
193
193
- ```C++
194
+ ``` cpp
194
195
std::string routeStart = " /redfish/v1" ;
195
196
196
197
BMCWEB_ROUTE (routestart + "/SessionService/")
@@ -203,9 +204,9 @@ by a simple grep statement to search for urls in question. Doing the above makes
203
204
the route handlers no longer greppable, and complicates bmcweb patchsets as a
204
205
whole.
205
206
206
- ### 11. Not responding to 404
207
+ ## 11. Not responding to 404
207
208
208
- ``` C++
209
+ ```cpp
209
210
BMCWEB_ROUTE("/myendpoint/<str>",
210
211
[](Request& req, Response& res, const std::string& id){
211
212
crow::connections::systemBus->async_method_call(
@@ -228,16 +229,16 @@ All bmcweb routes should handle 404 (not found) properly, and return it where
228
229
appropriate. 500 internal error is not a substitute for this, and should be only
229
230
used if there isn't a more appropriate error code that can be returned. This is
230
231
important, because a number of vulnerability scanners attempt injection attacks
231
- in the form of /myendpoint/foobar, or /myendpoint/#$*(%)&#%$)(\*& in an attempt
232
- to circumvent security. If the server returns 500 to any of these requests, the
233
- security scanner logs it as an error for followup. While in general these errors
234
- are benign, and not actually a real security threat, having a clean security run
235
- allows maintainers to minimize the amount of time spent triaging issues reported
236
- from these scanning tools.
232
+ in the form of ` /myendpoint/foobar ` , or ` /myendpoint/#$*(%)&#%$)(\*& ` in an
233
+ attempt to circumvent security. If the server returns 500 to any of these
234
+ requests, the security scanner logs it as an error for followup. While in
235
+ general these errors are benign, and not actually a real security threat, having
236
+ a clean security run allows maintainers to minimize the amount of time spent
237
+ triaging issues reported from these scanning tools.
237
238
238
239
An implementation of the above that handles 404 would look like:
239
240
240
- ```C++
241
+ ``` cpp
241
242
BMCWEB_ROUTE ("/myendpoint/<str >",
242
243
[ ] (Request& req, Response& res, const std::string& id){
243
244
crow::connections::systemBus->async_method_call(
@@ -265,9 +266,9 @@ on a working system, and any cases where 500 is found, can immediately be
265
266
assumed to be
266
267
[a bug in either the system, or bmcweb.](https://github.com/openbmc/bmcweb/blob/master/DEVELOPING.md#error-handling)
267
268
268
- ### 12. Imprecise matching
269
+ ## 12. Imprecise matching
269
270
270
- ``` C++
271
+ ```cpp
271
272
void isInventoryPath(const std::string& path){
272
273
if (path.find("inventory")){
273
274
return true;
@@ -281,7 +282,7 @@ avoid doing direct string containment matching. Doing so can lead to errors
281
282
where fan1 and fan11 both report to the same object, and cause behavior breaks
282
283
in subtle ways.
283
284
284
- When using dbus paths, rely on the methods on sdbusplus::message::object_path.
285
+ When using dbus paths, rely on the methods on ` sdbusplus::message::object_path ` .
285
286
When parsing HTTP field and lists, use the RFC7230 implementations from
286
287
boost::beast.
287
288
@@ -300,9 +301,9 @@ The above methods tend to be misused to accept user data and parse various
300
301
fields from it. In practice, there tends to be better, purpose built methods for
301
302
removing just the field you need.
302
303
303
- ### 13. Complete replacement of the response object
304
+ ## 13. Complete replacement of the response object
304
305
305
- ```
306
+ ``` cpp
306
307
void getMembers (crow::Response& res){
307
308
res.jsonValue = {{"Value", 2}};
308
309
}
@@ -314,7 +315,7 @@ Completely replacing the json object can lead to convoluted situations where the
314
315
output of the response is dependent on the _order_ of the asynchronous actions
315
316
completing, which cannot be guaranteed, and has many time caused bugs.
316
317
317
- ```
318
+ ```cpp
318
319
void getMembers(crow::Response& res){
319
320
res.jsonValue["Value"] = 2;
320
321
}
0 commit comments