# Checking the Code of Reiser4 with the PVS\-Studio Static Analyzer Hello there\! In this article, we'll look at the free version \(available to the developers of free and open\-source software\) of the PVS\-Studio static analyzer in action\. What we are going to check today is the source code of the Reiser4 file system and its utilities\. ![0551_Check_Reiser4/image1.png](https://import.viva64.com/docx/blog/0551_Check_Reiser4/image1.png) _This article was originally [posted](https://habr.com/en/post/345894/) on the Habrahabr website and reposted here by permission of the author\._ I hope all of you who are about to read this article have heard, if only in passing, about the static code analyzer PVS\-Studio\. If you haven't, follow [this](https://pvs-studio.com/en/pvs-studio/) link to read a brief product description\. The developer company also runs an official blog on [Habrahabr](https://habr.com/en/company/pvs-studio/) where they frequently post reports with the analysis results of various open\-source projects\. More information on Reiser4 can be found on the [kernel wiki page](https://reiser4.wiki.kernel.org/index.php/Why_Reiser4)\. Let's start with the Reiser4 utilities, specifically the libaal library\. Then we'll check the reiser4progs tools and round off with a review of the defects found in the kernel code\. ## Preparations We need to install PVS\-Studio to get started\. The official [website](https://pvs-studio.com/en/pvs-studio/download/) provides deb and rpm packages alongside an ordinary installation archive\. Choose whatever option is best for you\. Next, we need to activate the free license\. Open\-source software developers have to insert the following lines in the beginning of each source file \(it's not necessary to add them to header files\): _// This is an open source non\-commercial project\. Dear PVS\-Studio, please check it\._ _// PVS\-Studio Static Code Analyzer for C, C\+\+ and C\#: http://www\.viva64\.com_ Let's write a small bash script so that we don't have to repeat that process by hand for each file\. I'll use the sed stream editor to write the script \(the following instruction is written in one line\): ```cpp #!/usr/bin/bash for str in $(find $1 -name '*.c'); do sed -i -e '1 s/^/\/\/ This is an open source non-commercial project. Dear PVS-Studio, please check it.\n\/\/ PVS-Studio Static Code Analyzer for C, C++ and C\#: http:\/\/www.viva64.com\n\n/;' $str done ``` In addition, let's write another script to facilitate project building and PVS\-Studio launch: ```cpp #!/usr/bin/bash pvs-studio-analyzer trace -- make -j9 || exit 1 pvs-studio-analyzer analyze -o log.log -j9 || exit 1 plog-converter -a GA:1,2 -t tasklist log.log || exit 1 ``` We are ready to go\. The libaal library comes first\. ## Checking libaal\-1\.0\.7 libaal is a library that provides abstraction of Reiser4 structures and is used by reiser4progs\. **Analysis log**: [log1\.txt](http://cppfiles.com/Reiser4/log1.txt) If we agree to ignore the warnings dealing with redefinition of the standard data types, then possible bugs are found only in lines 68, 129, and 139 of the **src/bitops\.c** file: [V629](https://pvs-studio.com/en/docs/warnings/v629/) Consider inspecting the 'byte\_nr << 3' expression\. Bit shifting of the 32\-bit value with a subsequent expansion to the 64\-bit type\. Lines 129 and 139 contain the following code: ```cpp bit_t aal_find_next_set_bit(void *map, bit_t size, bit_t offset) { .... unsigned int byte_nr = offset >> 3; .... unsigned int nzb = aal_find_nzb(b, bit_nr); .... if (nzb < 8) return (byte_nr << 3) + nzb; .... } ``` This defect can be easily fixed by replacing the **unsigned int** type with **bit\_t** in the variable declarations\. As for line 68: ```cpp bit_t aal_find_first_zero_bit(void *map, bit_t size) { .... unsigned char *p = map; unsigned char *addr = map; .... return (p - addr) << 3; .... } ``` it's a mystery for me why PVS\-Studio believes the value of **\(p\-addr\)** to be 32\-bit\. Even **sizeof\(\)** yields the proper 8 bytes \(I'm working on amd64\)\. ## Checking reiser4progs\-1\.2\.1 **Analysis log**: [log2\.txt](http://cppfiles.com/Reiser4/log2.txt) Now, reiser4progs has much more interesting, and sometimes sadder, things to show\. By the way, here's what Edward Shishkin said about these tools: "The author left right after these progs were written, and no one has since looked into that code \(except for a couple of times when I was asked to fix fsck\)\. So I'm not surprised by that pile of bugs\." Indeed, it's no surprise that such specific bugs are still there after so many years\. The first serious error is found in the **plugin/key/key\_short/key\_short\_repair\.c** file: [V616](https://pvs-studio.com/en/docs/warnings/v616/) The 'KEY\_SHORT\_BAND\_MASK' named constant with the value of 0 is used in the bitwise operation\. ```cpp errno_t key_short_check_struct(reiser4_key_t *key) { .... if (oid & KEY_SHORT_BAND_MASK) key_short_set_locality(key, oid & !KEY_SHORT_BAND_MASK); .... } ``` **KEY\_SHORT\_BAND\_MASK** is the constant **0xf000000000000000ull**, which means the Boolean NOT operation produces _false _here \(in C, all values other than 0 are considered _true_\), i\.e\., in fact, 0\. However, the programmer obviously meant the bitwise NOT \(\~\) operation rather than the Boolean NOT\. This warning was triggered several times by different files\. Next comes **plugin/hash/tea\_hash/tea\_hash\.c** with errors such as this: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'len \>\= 16' is always false\. Wait\.\.\. It's not really an error \- it's some sort of black magic or a dirty trick \(if you don't believe in magic\)\. Why? Well, would you call the code below clear and straightforward without a deep understanding of the processor's and operating system's inner workings and the programmer's idea? ```cpp uint64_t tea_hash_build(unsigned char *name, uint32_t len) { .... while(len >= 16) { .... len -= 16; .... } .... if (len >= 12) { if (len >= 16) *(int *)0 = 0; .... } .... } ``` What'd you say? This is not an error, but we'd better leave this code alone unless we know what goes on here\. Let's try to figure it out\. The line **\*\(int \*\)0 \= 0;** would trigger a [**SIGSEGV**](https://en.wikipedia.org/wiki/Segmentation_fault) in a regular program\. If you search for information about the kernel, you'll find that this statement is used to make the kernel generate an **oops**\. This subject was discussed in the kernel developers' newsgroup \([here](https://www.spinics.net/lists/newbies/msg09413.html)\), and Torvalds himself [mentioned](https://yarchive.net/comp/linux/oops_decoding.html) it too\. So, if an assignment like that happens, in some mysterious ways, to execute inside the kernel code, you'll get an oops\. Why check for the "impossible" condition is something that only the author himself knows, but, as I said, we'd better let the thing be unless we know how it works\. The only thing we can safely investigate is why the V547 warning was triggered\. The **len \>\= 16** expression is always false\. The **while** loop is executed as long as the value of **len** is greater than or equal to 16, while the value 16 is subtracted at the end of the loop body at every iteration\. This means the variable can be represented as **len \= 16\*n\+m**, where **n **and** m** are integers and **m<16**\. It's obvious that once the loop is over, all the **16\*n**'s will have been subtracted, leaving only **m**\. The other warnings here follow the same pattern\. The following error is found in the **plugin/sdext/sdext\_plug/sdext\_plug\.c** file: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'stat' pointer was utilized before it was verified against nullptr\. Check lines: 18, 21\. ```cpp static void sdext_plug_info(stat_entity_t *stat) { .... stat->info.digest = NULL; if (stat->plug->p.id.id != SDEXT_PSET_ID || !stat) return; .... } ``` Either it's a banal typo or the author intended to write something else\. The **\!stat** check looks as if it were a **nullptr** check, but it doesn't make sense for two reasons\. Firstly, the **stat** pointer has been already dereferenced\. Secondly, this expression is evaluated from left to right, in accordance with the standard, so if it's really a **nullptr** check, it should be moved to the beginning of the condition since the pointer is originally dereferenced earlier in that same condition\. The **plugin/item/cde40/cde40\_repair\.c** file triggered a number of warnings such as this: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'pol \=\= 3' is always true\. ```cpp static errno_t cde40_pair_offsets_check(reiser4_place_t *place, uint32_t start_pos, uint32_t end_pos) { .... if (end_offset == cde_get_offset(place, start_pos, pol) + ENTRY_LEN_MIN(S_NAME, pol) * count) { return 0; } .... } ``` The programmer must have meant a construct of the **A \=\= \(B \+ C\)** pattern but inadvertently wrote it as **\(A \=\= B\) \+ C**\. **upd1\. It's my mistake; I confused the precedence of \+ and \=\=** The **plugin/object/sym40/sym40\.c** file contains a typo: [V593](https://pvs-studio.com/en/docs/warnings/v593/) Consider reviewing the expression of the 'A \= B < C' kind\. The expression is calculated as following: 'A \= \(B < C\)'\. ```cpp errno_t sym40_follow(reiser4_object_t *sym, reiser4_key_t *from, reiser4_key_t *key) { .... if ((res = sym40_read(sym, path, size) < 0)) goto error; .... } ``` This issue is similar to the previous one\. The **res** variable is assigned the result of a Boolean expression\. The programmer is obviously using a C "trick" here, so the expression should be rewritten as **\(A \= B\) < C**\. Another typo or mistake made from inattention\. File **libreiser4/flow\.c**: [V555](https://pvs-studio.com/en/docs/warnings/v555/) The expression 'end \- off \> 0' will work as 'end \!\= off'\. ```cpp int64_t reiser4_flow_write(reiser4_tree_t *tree, trans_hint_t *hint) { .... uint64_t off; uint64_t end; .... if (end - off > 0) { .... } .... } ``` There are two integer variables here\. Their difference is ALWAYS greater than or equal to zero because, from the viewpoint of how integers are represented in computer memory, subtraction and addition are, in effect, the same operation to the processor \([Two's complement](https://en.wikipedia.org/wiki/Two%27s_complement)\)\. The condition was more likely meant to check if **end \> off**\. Another probable typo: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'insert \> 0' is always true\. ```cpp errno_t reiser4_flow_convert(reiser4_tree_t *tree, conv_hint_t *hint) { .... for (hint->bytes = 0; insert > 0; insert -= conv) { .... if (insert > 0) { .... } .... } } ``` The code is contained in a loop, and the loop body is executed only when **insert \> 0**, so the condition is always true\. It's either a mistake, and therefore something else is missing, or a pointless check\. [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'ret' is always false\. ```cpp static errno_t repair_node_items_check(reiser4_node_t *node, place_func_t func, uint8_t mode, void *data) { .... if ((ret = objcall(&key, check_struct) < 0)) return ret; if (ret) { .... } .... } ``` The first condition contains a construct of the **A \= \( B < 0 \)** pattern, but what was more likely meant is **\(A \= B\) < C**\. The **librepair/semantic\.c** file seems to house another "black magic" thing: [V612](https://pvs-studio.com/en/docs/warnings/v612/) An unconditional 'break' within a loop\. ```cpp static reiser4_object_t *cb_object_traverse(reiser4_object_t *parent, entry_hint_t *entry, void *data) { .... while (sem->repair->mode == RM_BUILD && !attached) { .... break; } .... } ``` The **while** loop here is used as an **if** statement because the loop body will be executed only once \(since there's a **break** at the end\) if the condition is true or will be skipped otherwise\. Now guess what comes next? Exactly \- a typo\! The code is still looking like it was "abandoned at birth"\. This time, the problem is in the file **libmisc/profile\.c**: [V528](https://pvs-studio.com/en/docs/warnings/v528/) It is odd that pointer to 'char' type is compared with the '\\\\0' value\. Probably meant: \*c \+ 1 \=\= '\\\\0'\. ```cpp errno_t misc_profile_override(char *override) { .... char *entry, *c; .... if (c + 1 == '\0') { .... } .... } ``` Comparing a pointer with a terminal null character is a brilliant idea, no doubt, but the programmer more likely meant the check **\*\(c \+ 1\) \=\= '\\0'**, as the **\*c \+ 1 \=\= '\\0'** version doesn't make much sense\. Now let's discuss a couple of warnings dealing with the use of **fprintf\(\)**\. The messages themselves are straightforward, but we'll need to look in several files at once to understand what's going on\. First we'll peek into the **libmisc/ui\.c** file\. [V618](https://pvs-studio.com/en/docs/warnings/v618/) It's dangerous to call the 'fprintf' function in such a manner, as the line being passed could contain format specification\. The example of the safe code: printf\("%s", str\); Here's what we see: ```cpp void misc_print_wrap(void *stream, char *text) { char *string, *word; .... for (line_width = 0; (string = aal_strsep(&text, "\n")); ) { for (; (word = aal_strsep(&string, " ")); ) { if (line_width + aal_strlen(word) > screen_width) { fprintf(stream, "\n"); line_width = 0; } fprintf(stream, word); .... } .... } } ``` Let's find the code using this function\. Here it is, in the same file: ```cpp void misc_print_banner(char *name) { char *banner; .... if (!(banner = aal_calloc(255, 0))) return; aal_snprintf(banner, 255, BANNER); misc_print_wrap(stderr, banner); .... } ``` Now we're looking for **BANNER** \- it's in **include/misc/version\.h**: ```cpp #define BANNER \ "Copyright (C) 2001-2005 by Hans Reiser, " \ "licensing governed by reiser4progs/COPYING." ``` So, no injection danger\. Here's another issue of the same kind, this time in the files **progs/debugfs/browse\.c** and **progs/debugfs/print\.c**\. They employ the same code, so we'll discuss only **browse\.c**: ```cpp static errno_t debugfs_reg_cat(reiser4_object_t *object) { .... char buff[4096]; .... read = reiser4_object_read(object, buff, sizeof(buff)); if (read <= 0) break; printf(buff); .... } ``` Looking for the **reiser4\_object\_read\(\)** function: ```cpp int64_t reiser4_object_read( reiser4_object_t *object, /* object entry will be read from */ void *buff, /* buffer result will be stored in */ uint64_t n) /* buffer size */ { .... return plugcall(reiser4_psobj(object), read, object, buff, n); } ``` Finding out what **plugcall\(\)** does \- it turns out to be a macro: ```cpp /* Checks if @method is implemented in @plug and calls it. */ #define plugcall(plug, method, ...) ({ \ aal_assert("Method \""#method"\" isn't implemented " \ "in "#plug"", (plug)->method != NULL); \ (plug)->method(__VA_ARGS__); \ }) ``` Again, we need to find out what **method\(\)** does, and it, in its turn, depends on **plug**, and **plug** is **reiser4\_psobj\(object\)**: ```cpp #define reiser4_psobj(obj) \ ((reiser4_object_plug_t *)(obj)->info.pset.plug[PSET_OBJ]) ``` If we dig a bit deeper, we'll find that all of these are constant strings too: ```cpp char *pset_name[PSET_STORE_LAST] = { [PSET_OBJ] = "object", [PSET_DIR] = "directory", [PSET_PERM] = "permission", [PSET_POLICY] = "formatting", [PSET_HASH] = "hash", [PSET_FIBRE] = "fibration", [PSET_STAT] = "statdata", [PSET_DIRITEM] = "diritem", [PSET_CRYPTO] = "crypto", [PSET_DIGEST] = "digest", [PSET_COMPRESS] = "compress", [PSET_CMODE] = "compressMode", [PSET_CLUSTER] = "cluster", [PSET_CREATE] = "create", }; ``` Again, no injections possible\. The remaining issues are either errors of the same patterns as discussed above or defects that I don't think to be relevant\. ## Checking Reiser4 We have finally reached the Reiser4 code in the kernel\. To avoid building the entire kernel, let's modify the script we've written for launching PVS\-Studio to build only the code of Reiser4: ```cpp #!/usr/bin/bash pvs-studio-analyzer trace -- make SUBDIRS=fs/reiser4 -j9 || exit 1 pvs-studio-analyzer analyze -o log.log -j9 || exit 1 plog-converter -a GA:1,2 -t tasklist log.log || exit 1 ``` Thus we can have it build only the source code located in the folder **fs/reiser4**\. **Analysis log**: [log3\.txt](http://cppfiles.com/Reiser4/log3.txt) We'll ignore the warnings dealing with redefinition of the standard types in the headers of the kernel itself since the standard headers are not used in the build; and we are not interested in the kernel code anyway\. The first file to examine is **fs/reiser4/carry\.c**\. [V522](https://pvs-studio.com/en/docs/warnings/v522/) Dereferencing of the null pointer 'reference' might take place\. The null pointer is passed into 'add\_op' function\. Inspect the third argument\. Check lines: 564, 703\. ```cpp static carry_op *add_op(carry_level * level, /* &carry_level to add * node to */ pool_ordering order, /* where to insert: * at the beginning of @level; * before @reference; * after @reference; * at the end of @level */ carry_op * reference /* reference node for insertion */) { .... result = (carry_op *) reiser4_add_obj(&level->pool->op_pool, &level->ops, order, &reference->header); .... } ``` **reference** must be checked for **NULL** because later in the code, you can see the following call to the function declared above: ```cpp carry_op *node_post_carry(carry_plugin_info * info /* carry * parameters * passed down to node * plugin */ , carry_opcode op /* opcode of operation */ , znode * node /* node on which this * operation will operate */ , int apply_to_parent_p /* whether operation will * operate directly on @node * or on it parent. */ ) { .... result = add_op(info->todo, POOLO_LAST, NULL); .... } ``` where **add\_op\(\)** is explicitly called with the value of **reference** set to **NULL**, which results in an** oops**\. Next error: [V591](https://pvs-studio.com/en/docs/warnings/v591/) Non\-void function should return a value\. ```cpp static cmp_t carry_node_cmp(carry_level * level, carry_node * n1, carry_node * n2) { assert("nikita-2199", n1 != NULL); assert("nikita-2200", n2 != NULL); if (n1 == n2) return EQUAL_TO; while (1) { n1 = carry_node_next(n1); if (carry_node_end(level, n1)) return GREATER_THAN; if (n1 == n2) return LESS_THAN; } impossible("nikita-2201", "End of level reached"); } ``` This warning tells us that the function is non\-**void** and, therefore, must return some value\. The last line proves this is not an error because the case when **while** stops executing is an error\. [V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: \(result \=\= 0\)\. ```cpp int lock_carry_node(carry_level * level /* level @node is in */ , carry_node * node /* node to lock */) { .... result = 0; .... if (node->parent && (result == 0)) { .... } } ``` This is straightforward: the value of **result** doesn't change, so it's okay to omit the check\. [V1004](https://pvs-studio.com/en/docs/warnings/v1004/) The 'ref' pointer was used unsafely after it was verified against nullptr\. Check lines: 1191, 1210\. ```cpp carry_node *add_new_znode(znode * brother /* existing left neighbor * of new node */ , carry_node * ref /* carry node after which new * carry node is to be inserted * into queue. This affects * locking. */ , carry_level * doing /* carry queue where new node is * to be added */ , carry_level * todo /* carry queue where COP_INSERT * operation to add pointer to * new node will ne added */ ) { .... /* There is a lot of possible variations here: to what parent new node will be attached and where. For simplicity, always do the following: (1) new node and @brother will have the same parent. (2) new node is added on the right of @brother */ fresh = reiser4_add_carry_skip(doing, ref ? POOLO_AFTER : POOLO_LAST, ref); .... while (ZF_ISSET(reiser4_carry_real(ref), JNODE_ORPHAN)) { .... } .... } ``` What happens in this check is that **ref** is checked for **nullptr** by the ternary operator and then passed to the **reiser4\_carry\_real\(\)** function, where null\-pointer dereferencing may take place with no prior **nullptr** check\. However, that never happens\. Let's look into the **reiser4\_carry\_real\(\)** function: ```cpp znode *reiser4_carry_real(const carry_node * node) { assert("nikita-3061", node != NULL); return node->lock_handle.node; } ``` As you can see, the **node** pointer is checked for **nullptr** inside the function body, so everything is OK\. Next comes a probably incorrect check in the file **fs/reiser4/tree\.c**: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'child\-\>in\_parent\.item\_pos \+ 1 \!\= 0' is always true\. ```cpp int find_child_ptr(znode * parent /* parent znode, passed locked */ , znode * child /* child znode, passed locked */ , coord_t * result /* where result is stored in */ ) { .... if (child->in_parent.item_pos + 1 != 0) { .... } ``` We need to find the declaration of **item\_pos** to find out what exactly it is\. After searching in a few files we get the following: ```cpp struct znode { .... parent_coord_t in_parent; .... } __attribute__ ((aligned(16))); .... typedef struct parent_coord { .... pos_in_node_t item_pos; } parent_coord_t; .... typedef unsigned short pos_in_node_t; ``` In the comments, Andrey Karpov explained what this error is about\. The expression is cast to type **int **in the **if** statement, so no overflow will occur even if **item\_pos** is assigned the maximum value since casting the expression to **int** results in the value **0xFFFF \+ 1 \= 0x010000** rather than 0\. All the other bugs either follow one of the patterns discussed above or are false positives, which we have also talked about\. ## Conclusions They're simple\. Firstly, PVS\-Studio is cool\. A good tool helps you do your job better and faster if you know how to handle it\. As a static analyzer, PVS\-Studio has more than once proved to be a top\-level tool\. It provides you with the means to detect and solve hidden problems, typos, and mistakes\. Secondly, be careful writing code\. Don't use C "tricks" unless it's the only legal way to implement some feature\. Always use additional parentheses in conditions to explicitly state the desired order of calculations because even if you are a super\-duper hacker and C ace, you may simply confuse the operators' precedence and make a bunch of mistakes, especially when writing large portions of code at a time\. ## Acknowledgements: thanking the PVS\-Studio developers I'd like to thank the developers for such a wonderful tool\! They have done a really great job adapting PVS\-Studio to GNU/Linux systems and carefully designing the analyzer's implementation \(see the details [here](https://pvs-studio.com/en/blog/posts/0415/)\)\. It elegantly integrates into build systems and generates logs\. If you don't need integration, you can simply "intercept" compiler launches by running **make**\. And above all, thank you so much for giving students, open\-source projects, and single developers the opportunity to use your tool for free\! That's amazing\!