Reiser4 code check with PVS-Studio static analyzer

Good day!

This article is devoted to the use of the free version (for free and open source projects) of the PVS-Studio static analyzer. We will check the source code of the Reiser4 file system and its utilities.

I hope everyone who is going to read this article, even with the edge of his ear, heard about the PVS-Studio static code analyzer. If you are not one of them, then follow this link, where you can briefly read about this static analyzer.
')
Also, the developer company has an official blog on Habrahabr where reports on checking various open source projects often appear.

You can read a little about Reiser4 on the kernel wiki .
Perhaps we will start with the utilities, and specifically with the libaal library. Then we check the reiser4progs utilities, and we’ll leave the code check in the kernel for the last.

Preliminary preparation

First we need to install PVS-Studio. On the official site you can find the deb and rpm packages, as well as a simple archive with the program. We install in the most convenient way for us.

Next, you need to somehow use the free license. For open projects, it is necessary to add the following lines at the beginning of each source code file (optional in 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 

In order not to manually add these lines to each file, we will write a small script in bash. For these purposes, we use the sed text stream editor:

 #!/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 

For convenience, we will write another script to build and run PVS-Studio:

 #!/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 

Now we are ready to analyze the source code. Let's start with the libaal library.

Check libaal-1.0.7

libaal is a Reiser4 structure abstraction library used by reiser4progs.


Apart from the warnings related to the repeated declaration of standard data types, then we have possible problems only in lines 68, 129 and 139 in the src / bitops.c file :

V629 Consider inspecting the 'byte_nr << 3' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type.

In line 129 and 139, the code is as follows:

 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; <...> } 

In this case, the error can be easily corrected by replacing the declarations of the unsigned int type with the bit_t type.

Regarding line 68:

 bit_t aal_find_first_zero_bit(void *map, bit_t size) <...> unsigned char *p = map; unsigned char *addr = map; <...> return (p - addr) << 3; <...> } 

then here I guess why PVS suddenly considers (p-addr) 32-bit. Even sizeof () produces a clear 8 bytes (I use amd64).

Reiser4progs-1.2.1 check

But in reiser4progs everything is much more interesting and in some places sadder. In general, Edward Shishkin mentioned that: “after these programs were written, the author immediately quit, and since then no one has looked into this code (I only fixed fsck a couple of times at the request). So this whole crop of mistakes is not surprising. ” And the truth is not surprising that such errors for so many years have not been removed.

The first serious error appears in the plugin / key / key_short / key_short_repair.c file :

V616 The 'KEY_SHORT_BAND_MASK' named constant with the value of 0 is used in the bitwise operation.


 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 a constant 0xf000000000000000ull , i.e. the boolean operation of negation, in this case, gives a lie (in C all that is not 0 is true), i.e. in fact 0. Obviously, the author meant a bitwise operation NOT - ~ , and not a Boolean.
This error is repeated several times, in several different files.

This is followed by plugin / hash / tea_hash / tea_hash.c with errors like:

V547 Expression 'len >= 16' is always false.

But ... it’s not really a mistake, it’s some kind of magic, or a dirty hack, if you don’t believe in magic. Why? But judge for yourself whether such a code can be considered understandable and obvious without a deep knowledge of the processor, operating system and the original idea of ​​the author of the code?

 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; <...> } 

How do you? This is not a mistake, but not realizing what is happening here is better not to touch it. Let's try to figure it out.
Code * (int *) 0 = 0; in a normal program, will result in sigspev . If you look for information on the kernel, then such code is used in it so that the kernel can do the oops. Questions on this topic surfaced in the core developer mailing list here , and Torvalds himself mentioned this. Those. it turns out that if in some unknown way such an assignment is executed in the kernel code, it will be oops. The reasons for checking the “impossible” conditions remain on the developer’s conscience, but, as I mentioned above, you don’t understand, don’t touch.
The only thing that we can safely disassemble is the reason for the V547 triggering. The expression len> = 16 is always false. The while loop is executed while len is greater than or equal to 16, and at the end of the loop, 16 is subtracted at each iteration. Those. the variable can be represented as len = 16 * n + m , where n, m are integers, and m <16 . It becomes obvious that after the completion of the cycle, all 16 * n will be subtracted, and m will remain.
The rest of these warnings follow the same pattern.

In the plugin / sdext / sdext_plug / sdext_plug.c file we find the following error:

V595 The 'stat' pointer was utilized before it was verified against nullptr. Check lines: 18, 21.


 static void sdext_plug_info(stat_entity_t *stat) { <...> stat->info.digest = NULL; if (stat->plug->p.id.id != SDEXT_PSET_ID || !stat) return; <...> } 

There is either a banal typo, or the author had something else. Checking ! Stat looks like checking to nullptr , but it doesn't make sense for two reasons. First, the stat pointer has already been dereferenced. Secondly, according to the standard, this expression is calculated from the left, to the right, and if this is really a test for nullptr, then it must be moved to the beginning of the condition, because earlier in the same condition the pointer is dereferenced.
In the plugin / item / cde40 / cde40_repair.c file, there are several operations of the form:

V547 Expression 'pol == 3' is always true.


 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 author, most likely, had in mind a construction of the form A == (B + C) , but inadvertently received (A == B) + C.

upd1. My mistake, confused priority + and ==

In the plugin / object / sym40 / sym40.c file we encounter an error - a typo:

V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'.


 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; <...> } 

The problem is similar to the one that was higher. We see that the variable res is assigned the result of a boolean expression. Obviously, the “feature” C was applied here and the expression should be rewritten as (A = B) <C .

Another representative of typos or inattention. File libreiser4 / flow.c :

V555 The expression 'end - off > 0' will work as 'end != off'.


 int64_t reiser4_flow_write(reiser4_tree_t *tree, trans_hint_t *hint) { <...> uint64_t off; uint64_t end; <...> if (end - off > 0) { <...> } 

We have two integer variables. Their difference is ALWAYS greater than or equal to zero, because, from the point of view of representing integers in computer memory, for the processor, subtraction and addition are the essence of the same operation ( Additional Code ). Most likely the condition needs to be replaced with end> off .

Another possible error is a typo:

V547 Expression 'insert > 0' is always true.


 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 in a loop, and the body of the loop is executed only when insert> 0 , i.e. the check in the condition is always true in this part of the code. We have either a mistake and, as a result, something else was meant, or there is a useless check.

V547 Expression 'ret' is always false.


 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) { <...> } 

We see that in the previous condition a construction of the form A = (B <0) , and most likely (A = B) <C was meant.

In the librepair / semantic.c file, another representative of “black magic” is probably present:

V612 An unconditional 'break' within a loop.


 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; } <...> } 

In this case, the while loop is used as an if statement , since if the condition is true, then the body will be executed once (because there is a break at the end), or it will not be fulfilled in the case when the condition is false.

Try to guess the problem of which plan will appear before us next?
That's right, this is a typo - a mistake! The code continues to look "written and abandoned." This time there is an error in the libmisc / profile.c file:

V528 It is odd that pointer to 'char' type is compared with the '\\0' value. Probably meant: *c + 1 == '\\0'.


 errno_t misc_profile_override(char *override) { <...> char *entry, *c; <...> if (c + 1 == '\0') { <...> } 

Comparing a pointer with a terminal symbol is, without a doubt, a strong solution, but most likely it meant * (c + 1) == '\ 0' , since option * c + 1 == '\ 0' does not make much sense.

Consider a couple of warnings about using fprintf () . The warnings themselves are simple, but in order to see what is happening in them, you need to skip over several files.
To get started, let's get into the file libmisc / ui.c.

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);


We see in it the following code:

 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); <...> } 

We are looking for using this feature. We find in the same file:

 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); <...> } 

We are looking for BANNER and find it in the include / misc / version.h file:

 #define BANNER \ "Copyright (C) 2001-2005 by Hans Reiser, " \ "licensing governed by reiser4progs/COPYING." 

Those. no "injections" can occur.

Consider the second such error in the progs / debugfs / browse.c and progs / debugfs / print.c files they use the same code, so we only consider browse.c

 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); <...> } 

We are looking for the reiser4_object_read () function:

 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); } 

We are looking for something that makes plugcall () , and this is a macro:

 /* 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__); \ }) 

And once again you need to find something with the same method () . And it depends on the plug , and the plug is reiser4_psobj (object) :

 #define reiser4_psobj(obj) \ ((reiser4_object_plug_t *)(obj)->info.pset.plug[PSET_OBJ]) 

If you dig into the code, it turns out that all these are also constant strings:

 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", }; 

And no injections will work.

The remaining errors are either of the same plan as those discussed above, or those that in this case I see no reason to consider. For example, errors in the examples , or unsafe use of fprint (in theory, you can use format string injection ) . The examples are not very interesting to me , and the safe use of functions like fprintf () were considered more than once without me .

Reiser4 check

We proceed directly to the verification of the Reiser4 code in the kernel. In order not to build the entire kernel, we modify the script to start PVS so that only the Reiser4 code is built:

 #!/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 only have the source code located in the fs / reiser4 folder .


We skip the errors and warnings associated with overriding the standard types in the headers of the kernel itself, because the standard headers are not used in the build, and the kernel code is not interesting to us.

The first file in our path is fs / reiser4 / carry.c .

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.


 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); <...> } 

In this case, the reference check for NULL is necessary, since Further in the code you can find a similar call to this function:

 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 a reference value of NULL and the kernel will do oops .

The following error:

V591 Non-void function should return a value.


 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"); } 

The error indicates that the function is not void ; therefore, it must return some value. From the last line of code it becomes obvious that this case is not an error, because the case when while will cease to be executed is an error.

V560 A part of conditional expression is always true: (result == 0).


 int lock_carry_node(carry_level * level /* level @node is in */ , carry_node * node/* node to lock */) { <...> result = 0; <...> if (node->parent && (result == 0)) { <...> } 

Everything is simple, the result value does not change and the test can be omitted. Nothing wrong.

V1004 The 'ref' pointer was used unsafely after it was verified against nullptr. Check lines: 1191, 1210.


 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)) { <...> } 

The essence of this check is that in the ternary operator there is a check for ref on nullptr , and then it is passed to the function reiser4_carry_real () where the pointer is potentially dereferenced without checking for nullptr . However, it is not. Consider the function reiser4_carry_real () :

 znode *reiser4_carry_real(const carry_node * node) { assert("nikita-3061", node != NULL); return node->lock_handle.node; } 

We see that the node pointer is checked for nullptr in the function body, so that everything is in order.

This is followed by the possible incorrect triggering of the check in the file fs / reiser4 / tree.c :

V547 Expression 'child->in_parent.item_pos + 1 != 0' is always true.


 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) { <...> } 

You need to find the ad item_pos and understand what it is. Poryskav in several files, we obtain the following:

 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; 

We see that item_pos is integer, unsigned. Those. we have exactly one case where the condition is true - this is when item_pos is as large as possible and at +1 an overflow occurs and the variable eventually becomes zero, therefore the condition for zero inequality is false.

In the comments, Andrey2008 indicated what the error was here. And it consists in the fact that the if expression is converted to the int type, so even in the case of the maximum value of item_pos, there will be no overflow, since the expression is converted to the type int and instead of zero, we get the following 0xFFFF + 1 = 0x010000

All other errors are either similar to those considered above, or are false positives, which were also considered above.

findings

The conclusions are simple.
First of all, PVS-Studio is cool. Any good tool in the right hands allows you to work better and more productively. PVS-Studio as a static code analyzer has repeatedly shown itself from the best sides. It allows you to find and solve unexpected problems, typos or oversights by the developer.

Secondly, write the code more attentively. You do not need to use the "hacks" of C, at least not in those places where it is really justified and it cannot be done without it. In conditions, always use additional parentheses for prioritization, for even if you are a super-duper hacker and a C language specialist, you can simply forget the correct priorities of operators and get a lot of errors, especially if you write a lot of code in one go.

Thanks to the developers of PVS-Studio

Separately, I want to thank the developers for such a great tool! They did their best to implement PVS-Studio for GNU / Linux systems and think through the implementation of the analyzer (you can read more here ). It is elegantly embedded in the build system and generates logs. If you do not want to build in the build system, then you can simply “catch” the compiler starts by running make .

And most importantly, thank you so much for the opportunity to use for free for students, free projects and individual developers! It's fine!