summaryrefslogtreecommitdiff
path: root/COMMON_ERRORS.md
diff options
context:
space:
mode:
authorPatrick Williams <patrick@stwcx.xyz>2022-12-07 16:43:50 +0300
committerPatrick Williams <patrick@stwcx.xyz>2022-12-07 16:43:50 +0300
commitf4f2643ae8aa4a6e8f8db62a7e0377a8f222c3f9 (patch)
tree4aa50c1dfe42dc4f1ec58dcc176f95f5d057855a /COMMON_ERRORS.md
parentdfa3fdc3dc1045babc67ecd7974c4d997006d9c4 (diff)
downloadbmcweb-f4f2643ae8aa4a6e8f8db62a7e0377a8f222c3f9.tar.xz
markdownlint: fix all warnings
Signed-off-by: Patrick Williams <patrick@stwcx.xyz> Change-Id: I1402cbd84c916792ca2fc0ad0f34db661cbdfa72
Diffstat (limited to 'COMMON_ERRORS.md')
-rw-r--r--COMMON_ERRORS.md81
1 files changed, 41 insertions, 40 deletions
diff --git a/COMMON_ERRORS.md b/COMMON_ERRORS.md
index 317cef829c..6cb7340bfd 100644
--- a/COMMON_ERRORS.md
+++ b/COMMON_ERRORS.md
@@ -9,9 +9,9 @@ not-always-obvious ways, or impose a pattern that tends to cause hard to find
bugs, or bugs that appear later. Every one has been submitted to code review
multiple times.
-### 1. Directly dereferencing a pointer without checking for validity first
+## 1. Directly dereferencing a pointer without checking for validity first
-```C++
+```cpp
int myBadMethod(const nlohmann::json& j){
const int* myPtr = j.get_if<int>();
return *myPtr;
@@ -20,21 +20,21 @@ int myBadMethod(const nlohmann::json& j){
This pointer is not guaranteed to be filled, and could be a null dereference.
-### 2. String views aren't null terminated
+## 2. String views aren't null terminated
-```C++
+```cpp
int getIntFromString(const std::string_view s){
return std::atoi(s.data());
}
```
This will give the right answer much of the time, but has the possibility to
-fail when string_view is not null terminated. Use from_chars instead, which
+fail when `string_view` is not null terminated. Use `from_chars` instead, which
takes both a pointer and a length
-### 3. Not handling input errors
+## 3. Not handling input errors
-```C++
+```cpp
int getIntFromString(const std::string& s){
return std::atoi(s.c_str());
}
@@ -42,12 +42,12 @@ int getIntFromString(const std::string& s){
In the case where the string is not representable as an int, this will trigger
undefined behavior at system level. Code needs to check for validity of the
-string, ideally with something like from_chars, and return the appropriate error
-code.
+string, ideally with something like `from_chars`, and return the appropriate
+error code.
-### 4. Walking off the end of a string
+## 4. Walking off the end of a string
-```C++
+```cpp
std::string getFilenameFromPath(const std::string& path){
size_t index = path.find("/");
if (index != std::string::npos){
@@ -58,9 +58,9 @@ std::string getFilenameFromPath(const std::string& path){
}
```
-### 5. Using methods that throw (or not handling bad inputs)
+## 5. Using methods that throw (or not handling bad inputs)
-```C++
+```cpp
int myBadMethod(nlohmann::json& j){
return j.get<int>();
}
@@ -90,7 +90,7 @@ Commonly used methods that fall into this pattern:
- std::stol
- std::stoll
-#### Special note: JSON
+### Special note: JSON
`nlohmann::json::parse` by default
[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
to `replace`. Although `ignore` preserves content 1:1, `replace` is preferred
from a security point of view.
-#### Special note: Boost
+### Special note: Boost
there is a whole class of boost asio functions that provide both a method that
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.
- boost::asio::ip::tcp::acceptor::listen();
- boost::asio::ip::address::make_address();
-### 6. Blocking functions
+## 6. Blocking functions
bmcweb uses a single reactor for all operations. Blocking that reactor for any
amount of time causes all other operations to stop. The common blocking
@@ -138,7 +138,7 @@ system, the fact that most filesystem accesses are into tmpfs (and therefore
should be "fast" most of the time) and in general how little the filesystem is
used in practice.
-### 7. Lack of locking between subsequent calls
+## 7. Lack of locking between subsequent calls
While global data structures are discouraged, they are sometimes required to
store temporary state for operations that require it. Given the single threaded
@@ -146,7 +146,7 @@ nature of bmcweb, they are not required to be explicitly threadsafe, but they
must be always left in a valid state, and checked for other uses before
occupying.
-```C++
+```cpp
std::optional<std::string> currentOperation;
void firstCallbackInFlow(){
currentOperation = "Foo";
@@ -159,9 +159,9 @@ void secondCallbackInFlow(){
In the above case, the first callback needs a check to ensure that
currentOperation is not already being used.
-### 8. Wildcard reference captures in lambdas
+## 8. Wildcard reference captures in lambdas
-```
+```cpp
std::string x; auto mylambda = [&](){
x = "foo";
}
@@ -173,11 +173,12 @@ async bmcweb code. While capturing by reference can be useful, given how
difficult these types of bugs are to triage, bmcweb explicitly requires that all
code captures variables by name explicitly, and calls out each variable being
captured by value or by reference. The above prototypes would change to
-[&x]()... Which makes clear that x is captured, and its lifetime needs tracked.
+`[&x]()...` Which makes clear that x is captured, and its lifetime needs
+tracked.
-### 9. URLs should end in "/"
+## 9. URLs should end in "/"
-```C++
+```cpp
BMCWEB("/foo/bar");
```
@@ -188,9 +189,9 @@ the route ending in slash and the one without. This allows both URLs to be used
by users. While many specifications do not require this, it resolves a whole
class of bug that we've seen in the past.
-### 10. URLs constructed in aggregate
+## 10. URLs constructed in aggregate
-```C++
+```cpp
std::string routeStart = "/redfish/v1";
BMCWEB_ROUTE(routestart + "/SessionService/")
@@ -203,9 +204,9 @@ by a simple grep statement to search for urls in question. Doing the above makes
the route handlers no longer greppable, and complicates bmcweb patchsets as a
whole.
-### 11. Not responding to 404
+## 11. Not responding to 404
-```C++
+```cpp
BMCWEB_ROUTE("/myendpoint/<str>",
[](Request& req, Response& res, const std::string& id){
crow::connections::systemBus->async_method_call(
@@ -228,16 +229,16 @@ All bmcweb routes should handle 404 (not found) properly, and return it where
appropriate. 500 internal error is not a substitute for this, and should be only
used if there isn't a more appropriate error code that can be returned. This is
important, because a number of vulnerability scanners attempt injection attacks
-in the form of /myendpoint/foobar, or /myendpoint/#$*(%)&#%$)(\*& in an attempt
-to circumvent security. If the server returns 500 to any of these requests, the
-security scanner logs it as an error for followup. While in general these errors
-are benign, and not actually a real security threat, having a clean security run
-allows maintainers to minimize the amount of time spent triaging issues reported
-from these scanning tools.
+in the form of `/myendpoint/foobar`, or `/myendpoint/#$*(%)&#%$)(\*&` in an
+attempt to circumvent security. If the server returns 500 to any of these
+requests, the security scanner logs it as an error for followup. While in
+general these errors are benign, and not actually a real security threat, having
+a clean security run allows maintainers to minimize the amount of time spent
+triaging issues reported from these scanning tools.
An implementation of the above that handles 404 would look like:
-```C++
+```cpp
BMCWEB_ROUTE("/myendpoint/<str>",
[](Request& req, Response& res, const std::string& id){
crow::connections::systemBus->async_method_call(
@@ -265,9 +266,9 @@ on a working system, and any cases where 500 is found, can immediately be
assumed to be
[a bug in either the system, or bmcweb.](https://github.com/openbmc/bmcweb/blob/master/DEVELOPING.md#error-handling)
-### 12. Imprecise matching
+## 12. Imprecise matching
-```C++
+```cpp
void isInventoryPath(const std::string& path){
if (path.find("inventory")){
return true;
@@ -281,7 +282,7 @@ avoid doing direct string containment matching. Doing so can lead to errors
where fan1 and fan11 both report to the same object, and cause behavior breaks
in subtle ways.
-When using dbus paths, rely on the methods on sdbusplus::message::object_path.
+When using dbus paths, rely on the methods on `sdbusplus::message::object_path`.
When parsing HTTP field and lists, use the RFC7230 implementations from
boost::beast.
@@ -300,9 +301,9 @@ The above methods tend to be misused to accept user data and parse various
fields from it. In practice, there tends to be better, purpose built methods for
removing just the field you need.
-### 13. Complete replacement of the response object
+## 13. Complete replacement of the response object
-```
+```cpp
void getMembers(crow::Response& res){
res.jsonValue = {{"Value", 2}};
}
@@ -314,7 +315,7 @@ Completely replacing the json object can lead to convoluted situations where the
output of the response is dependent on the _order_ of the asynchronous actions
completing, which cannot be guaranteed, and has many time caused bugs.
-```
+```cpp
void getMembers(crow::Response& res){
res.jsonValue["Value"] = 2;
}