Skip to content

Commit

Permalink
markdownlint: fix all warnings
Browse files Browse the repository at this point in the history
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Change-Id: I1402cbd84c916792ca2fc0ad0f34db661cbdfa72
  • Loading branch information
williamspatrick committed Dec 7, 2022
1 parent dfa3fdc commit f4f2643
Show file tree
Hide file tree
Showing 13 changed files with 353 additions and 349 deletions.
2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/bug_report.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ assignees: ""
QEMU, please add

What specific OpenBMC versions (SHA1) did you use? (note, SHA1 should be
resolvable to https://github.com/openbmc/openbmc)
resolvable to <https://github.com/openbmc/openbmc>)

**To Reproduce** Steps to reproduce the behavior:

Expand Down
2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/custom.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ assignees: ""
Please do not use GitHub issues for submitting questions. Please submit your
questions either to the OpenBMC mailing list, or to the OpenBMC discord channel.

https://github.com/openbmc/docs/blob/master/README.md#contact
<https://github.com/openbmc/docs/blob/master/README.md#contact>
2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/feature_request.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ assignees: ""

Please do not submit feature requests to GitHub. Please indicate your interest
in particular projects or features on the mailing list at
https://lists.ozlabs.org/listinfo/openbmc
<https://lists.ozlabs.org/listinfo/openbmc>
3 changes: 3 additions & 0 deletions .markdownlint.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
default: true
MD024:
siblings_only: true
10 changes: 6 additions & 4 deletions CLIENTS.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# Client overview

bmcweb being a user and network facing daemon, is subject to a vast array of
tests and clients that could target it. The below attempts to provide a
non-exhaustive list of tests and clients that bmcweb is expected to be
Expand Down Expand Up @@ -31,13 +33,13 @@ portion of the OpenBMC Redfish use cases.
Status: Passing for some machines with CI integration.

slowloris: A tool to verify timeouts and DOS attack mitigation is implemented
properly. https://github.com/gkbrk/slowloris
properly. <https://github.com/gkbrk/slowloris>

Status: Passing, no automated enforcement.

testssl.sh: A tool for verifying the corectness of the bmcweb cipher suites
against current recommended security standards
https://github.com/drwetter/testssl.sh
<https://github.com/drwetter/testssl.sh>

Status: Unknown

Expand All @@ -55,7 +57,7 @@ git@github.com:DMTF/python-redfish-library.git
Status: Compatible

Redfish-Event-Listener: An example client for testing and implementing
EventService handlers. https://github.com/DMTF/Redfish-Event-Listener
EventService handlers. <https://github.com/DMTF/Redfish-Event-Listener>

Status: Compatible. No CI integration.

Expand All @@ -65,6 +67,6 @@ git@github.com:DMTF/Redfish-Tacklebox.git
Status: Unknown.

redfishtool: A generic command line tool for reading and writing operations to
the Redfish server. https://github.com/DMTF/Redfishtool
the Redfish server. <https://github.com/DMTF/Redfishtool>

Status: Compatible. No automated testing.
81 changes: 41 additions & 40 deletions COMMON_ERRORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -20,34 +20,34 @@ 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());
}
```
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){
Expand All @@ -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>();
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -138,15 +138,15 @@ 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
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";
Expand All @@ -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";
}
Expand All @@ -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");
```

Expand All @@ -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/")
Expand All @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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;
Expand All @@ -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.

Expand All @@ -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}};
}
Expand All @@ -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;
}
Expand Down
2 changes: 2 additions & 0 deletions DBUS_USAGE.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# dbus usage

The following are guidelines for bmcweb's use of DBus to construct Redfish
schemas:

Expand Down
Loading

0 comments on commit f4f2643

Please sign in to comment.