Revision control
Copy as Markdown
Other Tools
= RNP development guide
The following are a set of conventions and items that are relevant to
contributors.
== Contributing
=== Pull Requests
See also: https://github.com/thoughtbot/guides/tree/master/code-review[Thoughtbot’s Code Review guide]
Pull Requests should be used for any non-trivial changes. This presents
an opportunity for feedback and allows the CI tests to complete prior to
merging.
The `master` branch should generally always be in a buildable and
functional state.
Pull Requests should be:
* Focused. Do not include changes that are unrelated to the main purpose
of the PR.
* As small as possible. Sometimes large pull requests may be necessary
for adding complex features, but generally they should be kept as small
as possible to ensure a quick and thorough review process.
* Related to a GH issue to which you are assigned. If there is none,
file one (but search first!). This ensures there is no duplication of
effort and allows for a discussion prior to beginning work.
(This may not be necessary for PRs that are purely documentation updates)
* Approved by **2** reviewers before merging.
(Updates related to policies, like this section, should be approved by
the project owner)
* Merged by a reviewer via the most appropriate method
=== Branches
Git branches should be used generously. Most branches should be topic branches,
created for adding a specific feature or fixing a specific bug.
Keep branches short-lived (treat them as disposable/transient) and try to
avoid long-running branches.
A good example of using a branch would be:
* User `@joe` notices a bug where a NULL pointer is dereferenced during
key export. He creates GH issue `#500`.
* He creates a new branch to fix this bug named
`joe-500-fix-null-deref-in-pgp_export_key`.
* Joe commits a fix for the issue to this new branch.
* Joe creates a Pull Request to merge this branch in to main.
* Once merged, Joe deletes the branch since it is no longer useful.
Branch names may vary but should be somewhat descriptive, with words
separated by hyphens. It is also helpful to start the branch name with
your GitHub username, to make it clear who created the branch and
prevent naming conflicts.
Remember that branch names may be preserved permanently in the commit
history of `main`, depending on how they are merged.
=== Commits
* Try to keep commits as small as possible. This may be difficult or
impractical at times, so use your best judgement.
* Each commit should be buildable and should pass all tests. This helps
to ensure that git bisect remains a useful method of pinpointing issues.
* Commit messages should follow 50/72 rule.
* When integrating pull requests, merge function should be preferred over
squashing. From the other hand, developers should squash commits and
create meaningful commit stack before PR is merged into mainstream branch.
Merging commits like "Fix build" or "Implement comments from code review"
should be avoided.
== Continuous Integration (Github Actions)
Github actions are used for continuously testing new commits and pull requests.
Those include testing for different operating systems, linting via clang-format and shellcheck,
and code coverage and quality checks via `Codecov` and `LGTM.io`.
For Github workflows sources see `.github/workflows/` folder and scripts from the `ci/` folder.
Also there is a Cirrus CI runner, configuration for which is stored in `.cirrus.yml`.
=== Reproducing Locally
If tests fail in CI, you may attempt to reproduce those locally via `ctest` command:
[source,console]
--
ctest -j4 -V -R rnp_tests
--
Or, more specific:
[source,console]
--
ctest -V -R cli_tests-Misc
--
If test fails under the specific OS, you should construct corresponding Docker container and run tests inside, taking Github workflows as a guide.
== Code Coverage
CodeCov is used for assessing our test coverage.
== Security / Bug Hunting
=== Static Analysis
==== Coverity Scan
Coverity Scan is used for static analysis of the code base.
It is run daily on the main branch via the Github actions.
See `.github/workflows/coverity.yml` for the details.
You will need to create an account and request access to the rnpgp/rnp project.
Since the scan results are not updated live, line numbers may no longer
be accurate against the `main` branch, issues may already be resolved,
etc.
==== Clang Static Analyzer
Clang includes a useful static analyzer that can also be used to locate
potential bugs.
Note: It is normal for the build time to increase significantly when using this static analyzer.
[source,console]
--
# it's important to start fresh for this!
rm -rf build && mkdir build && cd build
scan-build cmake .. && scan-build make -j8
[...]
scan-build: 61 bugs found.
scan-build: Run 'scan-view /tmp/scan-build-2018-09-17-085354-22998-1' to examine bug reports.
--
Then use `scan-view`, as indicated above, to start a web server and use
your web browser to view the results.
=== Dynamic Analysis
==== Fuzzing
It is often useful to utilize a fuzzer like
ways to improve the robustness of the code base.
Presently, rnp builds in
and certain fuzzers are enabled there.
In the `src/fuzzing` directory, we have the fuzzers that run in OSS-Fuzz.
Setting `-DENABLE_SANITIZERS=1 -DENABLE_FUZZERS=1` will build these fuzzers
with the libfuzzer engine; and running the resulting executables will perform
the fuzzing.
To build and run fuzzers locally, or reproduce an issue, see https://google.github.io/oss-fuzz/advanced-topics/reproducing/
===== Further Reading
* AFL's `README`, `parallel_fuzzing.txt`, and other bundled documentation.
* See https://fuzzing-project.org/tutorial3.html[Tutorial: Instrumented fuzzing with american fuzzy lop]
==== Clang Sanitizer
Clang and GCC both support a number of sanitizers that can help locate
issues in the code base during runtime.
To use them, you should rebuild with the sanitizers enabled, and then
run the tests (or any executable):
[source,console]
--
env CXX=clang++ CXXFLAGS="-fsanitize=address,undefined" LDFLAGS="-fsanitize=address,undefined" ./configure
make -j4
src/tests/rnp_tests
--
Here we are using the
and
This will produce output showing any memory leaks, heap overflows, or
other issues.
== Code Conventions
C is a very flexible and powerful language. Because of this, it is
important to establish a set of conventions to avoid common problems and
to maintain a consistent code base.
=== Code Formatting
`clang-format` (v11.0.0) can be used to format the code base, utilizing
the `.clang-format` file included in the repository.
==== clang-format git hook
A git pre-commit hook exists to perform this task automatically, and can
be enabled like so:
[source,console]
--
cd rnp
git-hooks/enable.sh
--
If you do not have clang-format v11.0.0 available, you can use a docker
container for this purpose by setting `USE_DOCKER="yes"` in
`git-hooks/pre-commit.sh`.
This should generally work if you commit from the command line.
Note that if you have unstaged changes on some of the files you are
attempting to commit, which have formatting issues detected, you will
have to resolve this yourself (the script will inform you of this).
If your commit does not touch any `.c`/`.h` files, you can skip the
pre-commit hook with git's `--no-verify`/`-n` option.
==== clang-format (manually)
If you are not able to use the git hook, you can run `clang-format`
manually in a docker container.
Create a suitable container image with:
[source,console]
--
docker run --name=clang-format alpine:latest apk --no-cache add clang
docker commit clang-format clang-format
docker rm clang-format
--
You can then reformat a file (say, `src/lib/crypto/bn.cpp`) like so:
[source,console]
--
cd rnp
docker run --rm -v $PWD:/rnp -w /rnp clang-format clang-format -style=file -i src/lib/crypto/bn.cpp
--
Also you may wish to reformat all modified uncommitted files:
[source,console]
--
docker run --rm -v $PWD:/rnp -w /rnp clang-format clang-format -style=file -i `git ls-files -m |grep "\.\(c\|h\|cpp\)\$"`
--
...or files, modified since referenced commit, say `54c5476`:
[source,console]
--
docker run --rm -v $PWD:/rnp -w /rnp clang-format clang-format -style=file -i `git diff --name-only 54c5476..HEAD |grep "\.\(c\|h\|cpp\)\$"`
--
=== Style Guide
In order to keep the code base consistent, we should define and adhere
to a single style.
When in doubt, consult the existing code base.
==== Naming
The following are samples that demonstrate the style for naming
different things.
* Functions: `some_function`
* Variables: `some_variable`
* Filenames: `packet-parse.c` `packet-parse.h`
* Struct: `pgp_key_t`
* Typedefed Enums: `pgp_pubkey_alg_t`
* Enum Values: `PGP_PKA_RSA = 1`
* Constants (macro): `RNP_BUFSIZ`
==== General Guidelines
Do:
* Do use header guards (`#ifndef SOME_HEADER_H [...]`) in headers.
* Do use `sizeof(variable)`, rather than `sizeof(type)`. Or
`sizeof(*variable)` as appropriate.
* Do use commit messages that close GitHub issues automatically, when
applicable. `Fix XYZ. Closes #78.` See
* Do declare functions `static` when they do not need to be referenced
outside the current source file.
* Do always use braces for conditionals, even if the block only contains a
single statement.
+
[source,c]
--
if (something) {
return val;
}
--
* Do use a default failure (not success) value for `ret` variables. Example:
+
[source,c]
--
rnp_result_t ret = RNP_ERROR_GENERIC;
// ...
return ret;
--
Do not:
* Do not use the static storage class for local variables, *unless* they
are constant.
+
**Not OK**
+
[source,c]
--
int somefunc() {
static char buffer[256];
//...
}
--
+
**OK**
+
[source,c]
--
int somefunc() {
static const uint16_t some_data[] = {
0x00, 0x01, 0x02, //...
};
}
--
* Do not use `pragma`, and try to avoid `__attribute__` as well.
* Do not use uninitialized memory. Try to ensure your code will not cause any errors in valgrind and other memory checkers.
==== Documentation
Documentation is done in Doxygen comments format, which must be put in header files.
Exception are static or having only definition functions - it is not required to document them,
however if they are documented then this should be done in the source file and using the @private tag.
Comments should use doxygen markdown style, like the following example:
[source,c]
--
/** Some comments regarding the file purpose, like 'PGP packet parsing utilities'
* @file
*/
/** brief description of the sample function which does something, keyword 'brief' is omitted
* Which may be continued here
*
* After an empty line you may add detailed description in case it is needed. You may put
* details about the memory allocation, what happens if function fails and so on.
*
* @param param1 first parameter, null-terminated string which should not be NULL
* @param param2 integer, some number representing something
* @param size number of bytes available to store in buffer
* @param buffer buffer to store results, may be NULL. In this case size can be used to
* obtain the required buffer length
* @return 0 if operation succeeds, or error code otherwise. If operation succeeds then buffer
* is populated with the resulting data, and size contains the length of this data.
* if error code is E_BUF_TOOSMALL then size will contain the required size to store
* the result
**/
rnp_result_t
rnp_do_operation(const char *param1, const int param2, int *size, char *buffer);
--
== OpenPGP protocol specification
During development you'll need to reference OpenPGP protocol and related documents.
Here is the list of RFCs and Internet Drafts available at the moment:
* https://www.ietf.org/rfc/rfc1991.txt[RFC 1991]: PGP Message Exchange Formats. Now obsolete, but may have some historical interest.
* https://www.ietf.org/rfc/rfc4880.txt[RFC 4880]: OpenPGP Message Format. Latest RFC available at the moment, however has a lot of suggested changes via RFC 4880bis
* https://www.ietf.org/id/draft-ietf-openpgp-rfc4880bis-09.txt[RFC 4880bis-09]: OpenPGP Message Format. Latest suggested update to the RFC 4880.
More information sources:
* https://mailarchive.ietf.org/arch/browse/openpgp/[OpenPGP Working Group mailing list]. Here you can pick up all the latest discussions and suggestions regarding the update of RFC 4880
* https://gitlab.com/openpgp-wg/rfc4880bis[OpenPGP Working Group gitlab]. Latest work on RFC update is available here.
== Reviewers and Responsibility areas
The individuals are responsible for the following areas of `rnp`.
When submitting a Pull Request please seek reviews by whoever is
responsible according to this list.
General:
* Code style: @dewyatt
* Algorithms: @randombit, @dewyatt, @flowher, @catap, @ni4
* Performance: @catap, @ni4
* CLI: @ni4
* GnuPG compatibility: @MohitKumarAgniotri, @frank-trampe, @ni4
* Security Testing/Analysis: @MohitKumarAgniotri, @flowher
* Autotools: @randombit, @zgyarmati, @catap
Data formats:
* OpenPGP Packet: @randombit, @catap, @ni4
* Keystore: @catap
* JSON: @zgyarmati
* SSH: @ni4
Bindings:
* FFI: @dewyatt
* Ruby: @dewyatt
* Java/JNI: @catap
* Obj-C/Swift: @ni4
* Python: @dewyatt, @ni4
Platforms:
* RHEL/CentOS: @dewyatt
* BSD:
* Windows: @rrrooommmaaa
* macOS / iOS / Homebrew: @ni4
* Debian: @zgyarmati