Improving BIND 9 Code Quality
Surý, Ondřej
BIND 9 consists of a huge and old codebase. In this talk, I would like you to show all the available tools that we use on regular basis to improve, refactor and make the BIND 9 code safer. I'll show the examples of various Google/LLVM Sanitizers, cppcheck, LLVM scan-build and semantic patching using coccinelle.
 Hello, welcome everyone. I would like to present our next speaker, it's André Suri from ISE who will be talking about bind 9 code quality. I don't think this is actually a DNS talk, is it? Oh, no. Right, good luck. A little bit. So, just to get a feeling of the room, who knows bind? Who you are? Who uses bind? Okay, who is the developer here? Oh, so many. You're going to bind? No, no. I would know something about that. And whoever programs something about DNS, like DNS developers. Okay. So, this talk is more about like what we are doing to bind at the moment. So, it's not strictly about the DNS, it's more about the bind. And about the concurrent programming. So, my question would be who does threaded programming? Okay, quite a little hints. So, there's a couple of prerequisites for us in bind. And that's continuously integration because if you don't use the tools I'm going to show you on a regular basis, it doesn't make sense at all. So, if you ever use the tools, just integrate them into your continuous integration. And then you have to have the time to fix all the bugs. So, about bind, it's a 20 years plus old code base. So, you can imagine. It's pretty nice C99 code sometimes and it's getting better. It's multi-freaded. It uses locking. It has its own RW lock which have some advantages like it allows downgrade and upgrade. And since by 914, which is current stable, we are right now just before the release in 916 stable version, it uses the SCD atomics or the shims for in GCC or in Windows API. So, I don't know if you can actually see that but there are a couple of tools that we can use or we are already using. It's kind of built from Clank. I'm going to talk about a CPP check. There's also Clank ID which does different kind of checks. We are not using that yet. For the runtime analysis, there's a lot of synthesizers from GCC and a lot of VM which are very good. And there's some commercial tools like the Covertiscan which can be actually used for free for the course projects. There's something called Sonor Cloud I think but that's like very chatted. It represents all kind of weird stuff that's not even like it has a lot of false positives for us. And there's lookgoodtome.com which also looks very good. And the last tool we use for Bind is Cosinell. It's very good tool for refactoring large code bases. It's mainly being used for Linux kernel but we are using that too. So, what's the scan build? It's static code analyzer. It works by replacing the CC and CXX environment variable and it comes from the LLVM and Clank suite. So, the usage is quite simple. This is for the auto projects. You need to use it for configure and then when you are running make, you keep the CC variable set from the configure step. And the output is just one of the comets because we try to be more verbose than just fix back in the commit messages. So, this includes the real reports from the scan build. And it finds that like little stuff, the bind code base is quite good in regard to the scan build. So, it found like small stuff like the result is never used and so we fix that obviously. Then the CPP check, that's a different tool. It's independent projects also static code analyzer. It's more difficult to use if you use Autocon because it requires compilation database which is a JSON file with like which includes all the configuration options and C flags and LV flags and the path to files. And you need to build a compilation database. So, there are some tools that can build that automatically. I think CMake and others for Automake, it's more complicated. And it has different levels of strictness. The most strict level is crazy at the moment for bind. I'm not saying it refers false positive, it returns good stuff but you are not there yet. So, this is the usage. So, you run configure then you for make and Automake, you use tool called bare which is I think like short for built ear. So, it listens what goes on and then you run CPP check on the compile commands JSON that's the compilation database and it runs the scan on all your source files. So, it's important to run your project in like a debug mode or enable everything in the configure so it gets analyzed. And the output looks like this. So, it's this is all from the tool called CPP check dash HTML report and you are lucky because the CPP check 1.90 just came out and it found a new errors because we were CPP check clean before. So, this is from the CPP check 9.90. So, it found a new errors and then if you click the individual error it looks like this. So, it annotated your source code and it chose the path through the file and it's very good. So, if you are developers and not using the CPP check I invite you to do so because we like it quite a lot. So, the next tool the Cossinell it's a program matching and transformation engine and it has something like called semantic patches. The usage is that you call the command as patch then you pass it a patch the semantic patch it can work on git repositories and so there are some other small options. So, this is and it outputs the number of files it matches according to the rules and then if there's anything that needs to be patched then it outputs the patch to the standard output or you can like patch the files in place there's an option for that too. This is a very simple semantic patch. So, what this semantic patch does that if it finds an assertion for example if you have a switch command and there's a default which always asserts like this is like custom bind assert that it insists zero. So, if the code reaches the point it crashes with assertion. So, it adds ISC unreachable which is actually just macro to attribute that the end couldn't be reached. So, it adds the ISC unreachable to all occurrences of the insist where it's not already there so that the condition behind. I'm sorry for the unicode character it's actually the normal asterisk dash but the font kept like changing that and this is more complicated example. We made a recently we made a change to the bind allocator so ISC man get cannot return null if the allocation fails the bind will crash because if you don't have enough memory then you have different kind of problems anyway with the current allocators in modern operating systems. So, this tries to catch all the occurrences where there's a statement which is you declare a statement S and then expression V. So, if V like the variable is now and then falls the statement it just deletes the whole block of the variable and there are some it's quite smart it even can understand so if V is not now and there's a block and there's an L statement you can just keep the other block which is the condition on the first block on the on the right with the S1, S2. So, it can do all kinds of transformations so it understands the C code and it transforms the code properly so it's not like a like find and replace it understands the C code and can delete the whole statement or transform them into something else. Again, if you are working with old code base like bind is and you need to do extensive refactoring this is very cool tool and you should try it. So, there are some more examples how to do stuff with Cousinel there's a web page there's we have a directory with all the semantic patches we use in bind but they are very bind specific and there's a Cousinel array.org which is even more examples in a Git repository. So, why and here comes the fun part of all it's not so funny if you have to fix all the bugs. So, who understands the memory ordering? Oh, I'm lucky I can tell you whatever I want. On a platform with weak memory consistency. Okay, me neither. Well, I'm trying. So, and what about memory barriers? Like, do you know what's that? Okay, one, two. So, and bonus question, what's lock allusion? Would anybody know? Okay, I'm not going to talk about lock allusion but it's a feature on Intel processors maybe on AMD too where you can like not block things but when they collide you roll back the whole transactions in a CPU and memory and you actually do the locking so you can avoid some locking on a hardware. Very cool but I'm sure there are some bugs hidden in there especially with Intel. So, Fred Sanitizer. This is runtime analyzer so it doesn't analyze the static code but it analyzed like running code. It requires a good test suite to run or you can run in on your production if you are brave enough. And it checks for data races. If multiple threads access the same memory location, more text ordering so if you have like multiple locks in place and they are locked in different order from different threads so there's a possibility of deadlock and other subtle errors in the inter-thread programming. The usage is again quite simple. You just add a flag to C flags or LD flags. Well, you need both and for bind we need to, I told you we are using our own implementation of RWLock so Fred Sanitizer doesn't understand that so we need to use the standard pthread RWLock here. Then you build the program, then you export some extra options. Because bind is quite complex we need to increase the history size. There's like an amount of memory the threads in there are used to keep track of the memory locations. And some other options which make it more like cleaner because for example lock path, lock path T-SEN basically means that the output is locked in the separate file and it's not mixed in the standard output. Exit code again means like usually it exits with Exit code 66 but if you are running a test suite you usually don't want to stop the test suite because it failed. So there are some like options like this then you run a test suite, bind has both unit test and system test and then there's an output like this. So as you can see there are data arrays between the two threads and there are similar locations like ISE and M-Handle on-ref but there's a data arrays and it's quite interesting data arrays because this is the on-ref function. It uses reference counting and if the reference goes to zero which is the function ISE ref count decrement so if it's not zero yet then it returns but if it's zero then it delegates resources. It should be correct right? So why is there a data arrays especially in this function? So it probably uses the reference counting and then the free function which is called here which just delegates resource and that's it. It clears the resource and then the memput is basically the ISE version of free and still there's a data arrays. It looks like innocent in a quiz right? So the fix is to either destroy to the reference handle. Why? Because the ref count API uses the standard atomics and the decrement function uses the memory ordering release and the destroy function uses the order acquire and to actually understand this this is from the documentation. So if you don't use that there's no synchronization between the threads so the destroy function, the missing destroy function made one of the threads not to synchronize with the other in decrement and then there was a data arrays because of the missing release load and like okay so who now thinks understands memory ordering? I understand memory ordering and atomics more than I understood them a year ago but I'm still like another example. There's data arrays on the PowerPC64 reported by Redhead because they do have some interesting hardware and it's caused by our own RWLock and the proposed patch again changes the memory ordering from relaxed which doesn't cause any synchronization to acquire and the compare exchange operation again to acquire. On the synthetic benchmark it works. I just look at the function and I think that basically there's an error in our RWLock implementation but we haven't seen it because the usual hardware has a strong memory consistency and it doesn't manifest so it manifests only on the PowerPC64 because it has a weak memory consistency. Again, like so but I think that there's a lesson to be learned here so to my, I like the saying don't do your own crypto and I would add don't do your own locking unless you absolutely know what you're doing. The good news is that in a new version by 916 we are actually running a benchmark now with the system library RWLock and with some other tweaks we are over one million requests per second so just dropping our own RWLock implementation increases the performance because we changed the other parts of bind. So that's good news, we might be dropping the implementation at all. Another example, there's a comment, no one else has this socket so we can free it, right? And the fix is actually to lock the socket structure because again there was a bug which no one else has the socket except when you are on FreeBSD in a VM where on a hardware with a load of cores. It was very hard to reproduce but it was reported by the FreeBSD maintainer and thank you for all the work he does by reporting the bugs to us. But it was very weird bug, we tested like with a lot of cores on Linux but it only manifests on FreeBSD in VM. If anybody can say me why then I would be happy to listen. So another thing on FreeBSD, we added to our continuous integration, we added FreeBSD and Windows in September last year, we also added OpenBSD because if you don't run in the CI on every merge request then it's like it's not happening because the developers are going to ignore the reports that came asynchronously. And there was an intermittent failure in the FreeBSD system test job and it was a crash in the LibFret system library and it was an OpenSSL exit concurrency because OpenSSL 1.1 automatically destroys the resources. But if you have multiple threats running and shutting down the resources then one of the threats were destroying the mutex is held by the OpenSSL before the OpenSSL on exit function ran. So we had to request that like there's an internal task manager in bind so in the fatal function you have to request the exclusivity of a task manager so it finishes all the other stuff before it crashes. Again, it was very weird because we were like looking why it's crashing in the system library. Here's a quote from the documentation. The other type of the bugs is the lock order inversion and this is actually the cycle at the beginning is quite short. I have some like eight mutex long bugs in bind at the moment. They are not yet fixed. But it looks like mixing the mutex in a different order from different threats. Just an example how it looks. The most simple thing you can do is to convert the locking to because there's a lot of flags in structures so we just converted this flag removed to standard atomic, dropped locking at all and it's projected by the atomics. And now you have to get the memory ordering right. So are we there yet with the threat synthesizer? Not yet. So this is the current result on the bind master. There's like ten distinct warnings from the threat synthesizer but there's something like more than 200 different paths to the warning. There's a lot of duplicates like these six errors are probably the same one and in the lock ordering there's probably also a lot of duplicates. So it's not that much bugs but we are not there yet. It doesn't crash on a regular basis. It just sometimes something happens and we don't know why. And getting this clean means that we can run the threat synthesizer in a strict mode so if there's a new warning it will stop the merge request from being merged. But as I said we are not there yet. So thank you for listening. We have seven minutes for questions. If you have any questions, note about memory ordering please. I have several. Just a very quick one. Did you also enable more compiler warnings over time? Yeah we run with WOL, WM, WM, W extra all the time. And we also have in the CI we have multiple jobs so we run with Clang, we run with GCC and we run with different optimization options because sometimes it also if you run with minus O0 sometimes it reports stuff that's not real bug but it like I don't know. There was a code that if there was a variable zero it didn't run a loop so on a higher level of optimization that got optimized out but on a lower level optimization it printed warning about something. So again like trying different optimization options from minus O0 to minus O3 is also a good option in the CI because the compilers do a different stuff. Also running the threat sanitizer with Clang and GCC various different results. So that's also thing you should do if you are running the sanitizers just do the different compilers because they created a little bit different code. Let's see your question. Just talk loud. I don't know what is our code coverage at the moment. You go back like 30 slides. I would like to ask you about C++ check. You use this tool to try to find bugs or exceptions in your code. Is it possible to use it for checking coding style? Well, Bind is written plain C so but I don't know if it's a C++ check. CPP check. Yeah, you mean this one. Is it possible to check just coding style? Some, yes. On some strictness level it also reports like variable scoping so when you can reduce the scope of the variable and some others. It's quite extensive list actually but it would be most to just show it here. This is what we use so we, this is from our CI line so we are just using these. I'm in the EMEX camp and I do my coding in EMEX so I use fly check to run CPP check and clank on my code so it automatically highlights the issues. I'm probably sure that the stuff that the young people use, all the ID so it does as well but EMEX, I like EMEX. So what's bear make? Bear make? It creates the completion database from right. Okay. Any other questions? So are you finding that CI is very expensive or slows down your team? Because I found that those are the options. So right now we are using hosted bare metal servers and we have like eight of them to run all the CI but we have some like 56 jobs at the moment. So it's getting slower and slower as we had more and more tests. We also have a Windows in the CI so it's just consumed one of the machines but the other machines are shared so if you are open BSD is ran in the QMU, free BSD had also consumed its own machine and it uses Jails. We wrote a plugin for GitLab CI to use Jails in the GitLab CI and also the open BSD that's also our own plugin. For Linux it uses the standard Docker but for the other systems we had to write custom executors for the GitLab CI. So define expensive. So if it catches back, it would cause a CVE later. Was that expensive? No, no, no. I found in some teams that it can scare people off. Like you say we are going to spend a thousand dollars a month now on just testing the code all the time. They say what can we do 100 and well yes then the team can do one feature a week. Yeah we are spending something under a hundred thousand dollars per month. Hundred thousand? No, thousand. Under thousand. Okay. One thousand. One thousand. Under one thousand a month. Under one thousand a month. It seems like a good deal. All right, thank you.
 Hello, welcome everyone. I would like to present our next speaker, it's André Suri from ISE  who will be talking about bind 9 code quality. I don't think this is actually a DNS talk,  is it?  Oh, no.  Right, good luck.  A little bit. So, just to get a feeling of the room, who knows bind? Who you are?  Who uses bind? Okay, who is the developer here? Oh, so many.  You're going to bind?  No, no. I would know something about that. And whoever programs something about DNS,  like DNS developers. Okay. So, this talk is more about like what we are doing to bind  at the moment. So, it's not strictly about the DNS, it's more about the bind. And about  the concurrent programming. So, my question would be who does threaded programming? Okay,  quite a little hints. So, there's a couple of prerequisites for us in bind. And that's  continuously integration because if you don't use the tools I'm going to show you on a regular  basis, it doesn't make sense at all. So, if you ever use the tools, just integrate them  into your continuous integration. And then you have to have the time to fix all the bugs.  So, about bind, it's a 20 years plus old code base. So, you can imagine. It's pretty  nice C99 code sometimes and it's getting better. It's multi-freaded. It uses locking. It has  its own RW lock which have some advantages like it allows downgrade and upgrade. And since  by 914, which is current stable, we are right now just before the release in 916 stable version,  it uses the SCD atomics or the shims for in GCC or in Windows API. So, I don't know if  you can actually see that but there are a couple of tools that we can use or we are already  using. It's kind of built from Clank. I'm going to talk about a CPP check. There's also Clank  ID which does different kind of checks. We are not using that yet. For the runtime analysis,  there's a lot of synthesizers from GCC and a lot of VM which are very good. And there's  some commercial tools like the Covertiscan which can be actually used for free for the  course projects. There's something called Sonor Cloud I think but that's like very chatted.  It represents all kind of weird stuff that's not even like it has a lot of false positives  for us. And there's lookgoodtome.com which also looks very good. And the last tool we  use for Bind is Cosinell. It's very good tool for refactoring large code bases. It's mainly  being used for Linux kernel but we are using that too. So, what's the scan build? It's  static code analyzer. It works by replacing the CC and CXX environment variable and it  comes from the LLVM and Clank suite. So, the usage is quite simple. This is for the auto  projects. You need to use it for configure and then when you are running make, you keep  the CC variable set from the configure step. And the output is just one of the comets because  we try to be more verbose than just fix back in the commit messages. So, this includes  the real reports from the scan build. And it finds that like little stuff, the bind code  base is quite good in regard to the scan build. So, it found like small stuff like the result  is never used and so we fix that obviously. Then the CPP check, that's a different tool.  It's independent projects also static code analyzer. It's more difficult to use if you  use Autocon because it requires compilation database which is a JSON file with like which  includes all the configuration options and C flags and LV flags and the path to files.  And you need to build a compilation database. So, there are some tools that can build that  automatically. I think CMake and others for Automake, it's more complicated. And it has  different levels of strictness. The most strict level is crazy at the moment for bind. I'm  not saying it refers false positive, it returns good stuff but you are not there yet. So,  this is the usage. So, you run configure then you for make and Automake, you use tool called  bare which is I think like short for built ear. So, it listens what goes on and then you  run CPP check on the compile commands JSON that's the compilation database and it runs  the scan on all your source files. So, it's important to run your project in like a debug  mode or enable everything in the configure so it gets analyzed. And the output looks like  this. So, it's this is all from the tool called CPP check dash HTML report and you are lucky  because the CPP check 1.90 just came out and it found a new errors because we were CPP  check clean before. So, this is from the CPP check 9.90. So, it found a new errors and  then if you click the individual error it looks like this. So, it annotated your source code  and it chose the path through the file and it's very good. So, if you are developers  and not using the CPP check I invite you to do so because we like it quite a lot. So,  the next tool the Cossinell it's a program matching and transformation engine and it  has something like called semantic patches. The usage is that you call the command as  patch then you pass it a patch the semantic patch it can work on git repositories and so  there are some other small options. So, this is and it outputs the number of files it matches  according to the rules and then if there's anything that needs to be patched then it  outputs the patch to the standard output or you can like patch the files in place there's  an option for that too. This is a very simple semantic patch. So, what this semantic patch  does that if it finds an assertion for example if you have a switch command and there's a  default which always asserts like this is like custom bind assert that it insists zero.  So, if the code reaches the point it crashes with assertion. So, it adds ISC unreachable  which is actually just macro to attribute that the end couldn't be reached. So, it adds  the ISC unreachable to all occurrences of the insist where it's not already there so  that the condition behind. I'm sorry for the unicode character it's actually the normal  asterisk dash but the font kept like changing that and this is more complicated example.  We made a recently we made a change to the bind allocator so ISC man get cannot return  null if the allocation fails the bind will crash because if you don't have enough memory  then you have different kind of problems anyway with the current allocators in modern operating  systems. So, this tries to catch all the occurrences where there's a statement which is you  declare a statement S and then expression V. So, if V like the variable is now and then  falls the statement it just deletes the whole block of the variable and there are some it's  quite smart it even can understand so if V is not now and there's a block and there's  an L statement you can just keep the other block which is the condition on the first  block on the on the right with the S1, S2. So, it can do all kinds of transformations  so it understands the C code and it transforms the code properly so it's not like a like  find and replace it understands the C code and can delete the whole statement or transform  them into something else. Again, if you are working with old code base like bind is and  you need to do extensive refactoring this is very cool tool and you should try it. So,  there are some more examples how to do stuff with Cousinel there's a web page there's we  have a directory with all the semantic patches we use in bind but they are very bind specific  and there's a Cousinel array.org which is even more examples in a Git repository. So, why  and here comes the fun part of all it's not so funny if you have to fix all the bugs. So,  who understands the memory ordering? Oh, I'm lucky I can tell you whatever I want. On a  platform with weak memory consistency. Okay, me neither. Well, I'm trying. So, and what  about memory barriers? Like, do you know what's that? Okay, one, two. So, and bonus question,  what's lock allusion? Would anybody know? Okay, I'm not going to talk about lock allusion  but it's a feature on Intel processors maybe on AMD too where you can like not block things  but when they collide you roll back the whole transactions in a CPU and memory and you actually  do the locking so you can avoid some locking on a hardware. Very cool but I'm sure there  are some bugs hidden in there especially with Intel. So, Fred Sanitizer. This is runtime  analyzer so it doesn't analyze the static code but it analyzed like running code. It requires  a good test suite to run or you can run in on your production if you are brave enough.  And it checks for data races. If multiple threads access the same memory location,  more text ordering so if you have like multiple locks in place and they are locked in different  order from different threads so there's a possibility of deadlock and other subtle errors  in the inter-thread programming. The usage is again quite simple. You just add a flag  to C flags or LD flags. Well, you need both and for bind we need to, I told you we are  using our own implementation of RWLock so Fred Sanitizer doesn't understand that so we need  to use the standard pthread RWLock here. Then you build the program, then you export some extra  options. Because bind is quite complex we need to increase the history size. There's  like an amount of memory the threads in there are used to keep track of the memory locations.  And some other options which make it more like cleaner because for example lock path,  lock path T-SEN basically means that the output is locked in the separate file and it's not  mixed in the standard output. Exit code again means like usually it exits with Exit code  66 but if you are running a test suite you usually don't want to stop the test suite  because it failed. So there are some like options like this then you run a test suite,  bind has both unit test and system test and then there's an output like this. So as you  can see there are data arrays between the two threads and there are similar locations like  ISE and M-Handle on-ref but there's a data arrays and it's quite interesting data arrays  because this is the on-ref function. It uses reference counting and if the reference goes  to zero which is the function ISE ref count decrement so if it's not zero yet then it  returns but if it's zero then it delegates resources. It should be correct right? So why  is there a data arrays especially in this function? So it probably uses the reference  counting and then the free function which is called here which just delegates resource  and that's it. It clears the resource and then the memput is basically the ISE version  of free and still there's a data arrays. It looks like innocent in a quiz right? So the  fix is to either destroy to the reference handle. Why? Because the ref count API uses  the standard atomics and the decrement function uses the memory ordering release and the  destroy function uses the order acquire and to actually understand this this is from  the documentation. So if you don't use that there's no synchronization between the threads  so the destroy function, the missing destroy function made one of the threads not to  synchronize with the other in decrement and then there was a data arrays because of the  missing release load and like okay so who now thinks understands memory ordering?  I understand memory ordering and atomics more than I understood them a year ago but I'm  still like another example. There's data arrays on the PowerPC64 reported by Redhead because  they do have some interesting hardware and it's caused by our own RWLock and the proposed  patch again changes the memory ordering from relaxed which doesn't cause any synchronization  to acquire and the compare exchange operation again to acquire. On the synthetic benchmark  it works. I just look at the function and I think that basically there's an error in  our RWLock implementation but we haven't seen it because the usual hardware has a strong  memory consistency and it doesn't manifest so it manifests only on the PowerPC64 because  it has a weak memory consistency. Again, like so but I think that there's a lesson to be  learned here so to my, I like the saying don't do your own crypto and I would add don't do  your own locking unless you absolutely know what you're doing. The good news is that in  a new version by 916 we are actually running a benchmark now with the system library RWLock  and with some other tweaks we are over one million requests per second so just dropping  our own RWLock implementation increases the performance because we changed the other parts  of bind. So that's good news, we might be dropping the implementation at all. Another  example, there's a comment, no one else has this socket so we can free it, right? And  the fix is actually to lock the socket structure because again there was a bug which no one  else has the socket except when you are on FreeBSD in a VM where on a hardware with a  load of cores. It was very hard to reproduce but it was reported by the FreeBSD  maintainer and thank you for all the work he does by reporting the bugs to us. But it  was very weird bug, we tested like with a lot of cores on Linux but it only manifests  on FreeBSD in VM. If anybody can say me why then I would be happy to listen. So another  thing on FreeBSD, we added to our continuous integration, we added FreeBSD and Windows  in September last year, we also added OpenBSD because if you don't run in the CI on every  merge request then it's like it's not happening because the developers are going to ignore  the reports that came asynchronously. And there was an intermittent failure in the FreeBSD  system test job and it was a crash in the LibFret system library and it was an OpenSSL  exit concurrency because OpenSSL 1.1 automatically destroys the resources. But if you have multiple  threats running and shutting down the resources then one of the threats were destroying the  mutex is held by the OpenSSL before the OpenSSL on exit function ran. So we had to request  that like there's an internal task manager in bind so in the fatal function you have  to request the exclusivity of a task manager so it finishes all the other stuff before  it crashes. Again, it was very weird because we were like looking why it's crashing in  the system library. Here's a quote from the documentation. The other type of the bugs  is the lock order inversion and this is actually the cycle at the beginning is quite short.  I have some like eight mutex long bugs in bind at the moment. They are not yet fixed.  But it looks like mixing the mutex in a different order from different threats. Just an example  how it looks. The most simple thing you can do is to convert the locking to because there's  a lot of flags in structures so we just converted this flag removed to standard atomic, dropped  locking at all and it's projected by the atomics. And now you have to get the memory ordering  right. So are we there yet with the threat synthesizer? Not yet. So this is the current  result on the bind master. There's like ten distinct warnings from the threat synthesizer  but there's something like more than 200 different paths to the warning. There's a lot of duplicates  like these six errors are probably the same one and in the lock ordering there's probably  also a lot of duplicates. So it's not that much bugs but we are not there yet. It doesn't  crash on a regular basis. It just sometimes something happens and we don't know why. And  getting this clean means that we can run the threat synthesizer in a strict mode so if  there's a new warning it will stop the merge request from being merged. But as I said we  are not there yet. So thank you for listening. We have seven minutes for questions. If you  have any questions, note about memory ordering please. I have several. Just a very quick one.  Did you also enable more compiler warnings over time? Yeah we run with WOL, WM, WM,  W extra all the time. And we also have in the CI we have multiple jobs so we run with  Clang, we run with GCC and we run with different optimization options because sometimes it also  if you run with minus O0 sometimes it reports stuff that's not real bug but it like I don't  know. There was a code that if there was a variable zero it didn't run a loop so on a  higher level of optimization that got optimized out but on a lower level optimization it printed  warning about something. So again like trying different optimization options from minus O0  to minus O3 is also a good option in the CI because the compilers do a different stuff.  Also running the threat sanitizer with Clang and GCC various different results. So that's  also thing you should do if you are running the sanitizers just do the different compilers  because they created a little bit different code.  Let's see your question. Just talk loud.  I don't know what is our code coverage at the moment.  You go back like 30 slides.  I would like to ask you about C++ check. You use this tool to try to find bugs or exceptions  in your code. Is it possible to use it for checking coding style?  Well, Bind is written plain C so but I don't know if it's a C++ check.  CPP check.  Yeah, you mean this one. Is it possible to check just coding style?  Some, yes. On some strictness level it also reports like variable scoping so when you  can reduce the scope of the variable and some others. It's quite extensive list actually  but it would be most to just show it here. This is what we use so we, this is from our  CI line so we are just using these.  I'm in the EMEX camp and I do my coding in EMEX so I use fly check to run CPP check  and clank on my code so it automatically highlights the issues.  I'm probably sure that the stuff that the young people use, all the ID so it does as well  but EMEX, I like EMEX.  So what's bear make? Bear make? It creates the completion database from right.  Okay. Any other questions?  So are you finding that CI is very expensive or slows down your team?  Because I found that those are the options.  So right now we are using hosted bare metal servers and we have like eight of them to run  all the CI but we have some like 56 jobs at the moment.  So it's getting slower and slower as we had more and more tests.  We also have a Windows in the CI so it's just consumed one of the machines but the other  machines are shared so if you are open BSD is ran in the QMU, free BSD had also consumed  its own machine and it uses Jails.  We wrote a plugin for GitLab CI to use Jails in the GitLab CI and also the open BSD that's  also our own plugin.  For Linux it uses the standard Docker but for the other systems we had to write custom  executors for the GitLab CI.  So define expensive.  So if it catches back, it would cause a CVE later.  Was that expensive?  No, no, no.  I found in some teams that it can scare people off.  Like you say we are going to spend a thousand dollars a month now on just testing the code  all the time.  They say what can we do 100 and well yes then the team can do one feature a week.  Yeah we are spending something under a hundred thousand dollars per month.  Hundred thousand?  No, thousand.  Under thousand.  Okay.  One thousand.  One thousand.  Under one thousand a month.  Under one thousand a month.  It seems like a good deal.  All right, thank you.
dns_bind9_codequality_0_131.png dns_bind9_codequality_132_371.png dns_bind9_codequality_372_419.png dns_bind9_codequality_420_2351.png dns_bind9_codequality_2352_3395.png dns_bind9_codequality_3396_5243.png dns_bind9_codequality_5244_8171.png dns_bind9_codequality_8172_9203.png dns_bind9_codequality_9204_10151.png dns_bind9_codequality_10152_10355.png dns_bind9_codequality_10356_10655.png dns_bind9_codequality_10656_14123.png dns_bind9_codequality_14124_15167.png dns_bind9_codequality_15168_15947.png dns_bind9_codequality_15948_21647.png dns_bind9_codequality_21648_22127.png dns_bind9_codequality_22128_23255.png dns_bind9_codequality_23256_25115.png dns_bind9_codequality_25116_26723.png dns_bind9_codequality_26724_27335.png dns_bind9_codequality_27336_28331.png dns_bind9_codequality_28332_28571.png dns_bind9_codequality_28572_32135.png dns_bind9_codequality_32136_35039.png dns_bind9_codequality_35040_37199.png dns_bind9_codequality_37200_37379.png dns_bind9_codequality_37380_37955.png dns_bind9_codequality_37956_38891.png dns_bind9_codequality_38892_39095.png dns_bind9_codequality_39096_39167.png dns_bind9_codequality_39168_40115.png dns_bind9_codequality_40116_40415.png dns_bind9_codequality_40416_40487.png dns_bind9_codequality_40488_40823.png dns_bind9_codequality_40824_41531.png dns_bind9_codequality_41532_42575.png dns_bind9_codequality_42576_44567.png dns_bind9_codequality_44568_45131.png dns_bind9_codequality_45132_45227.png dns_bind9_codequality_45228_45346.png
Hello, welcome everyone. I would like to present our next speaker, it's André Suri from ISE who will be talking about bind 9 code quality. I don't think this is actually a DNS talk, is it? Oh, no. Right, good luck. A little bit. So, just to get a feeling of the room, who knows bind? Who you are? Who uses bind? Okay, who is the developer here? Oh, so many. You're going to bind? No, no. I would know something about that. And whoever programs something about DNS, like DNS developers. Okay. So, this talk is more about like what we are doing to bind at the moment. So, it's not strictly about the DNS, it's more about the bind. And about the concurrent programming. So, my question would be who does threaded programming? Okay, quite a little hints. So, there's a couple of prerequisites for us in bind. And that's continuously integration because if you don't use the tools I'm going to show you on a regular basis, it doesn't make sense at all. So, if you ever use the tools, just integrate them into your continuous integration. And then you have to have the time to fix all the bugs. So, about bind, it's a 20 years plus old code base. So, you can imagine. It's pretty nice C99 code sometimes and it's getting better. It's multi-freaded. It uses locking. It has its own RW lock which have some advantages like it allows downgrade and upgrade. And since by 914, which is current stable, we are right now just before the release in 916 stable version, it uses the SCD atomics or the shims for in GCC or in Windows API. So, I don't know if you can actually see that but there are a couple of tools that we can use or we are already using. It's kind of built from Clank. I'm going to talk about a CPP check. There's also Clank ID which does different kind of checks. We are not using that yet. For the runtime analysis, there's a lot of synthesizers from GCC and a lot of VM which are very good. And there's some commercial tools like the Covertiscan which can be actually used for free for the course projects. There's something called Sonor Cloud I think but that's like very chatted. It represents all kind of weird stuff that's not even like it has a lot of false positives for us. And there's lookgoodtome.com which also looks very good. And the last tool we use for Bind is Cosinell. It's very good tool for refactoring large code bases. It's mainly being used for Linux kernel but we are using that too. So, what's the scan build? It's static code analyzer. It works by replacing the CC and CXX environment variable and it comes from the LLVM and Clank suite. So, the usage is quite simple. This is for the auto projects. You need to use it for configure and then when you are running make, you keep the CC variable set from the configure step. And the output is just one of the comets because we try to be more verbose than just fix back in the commit messages. So, this includes the real reports from the scan build. And it finds that like little stuff, the bind code base is quite good in regard to the scan build. So, it found like small stuff like the result is never used and so we fix that obviously. Then the CPP check, that's a different tool. It's independent projects also static code analyzer. It's more difficult to use if you use Autocon because it requires compilation database which is a JSON file with like which includes all the configuration options and C flags and LV flags and the path to files. And you need to build a compilation database. So, there are some tools that can build that automatically. I think CMake and others for Automake, it's more complicated. And it has different levels of strictness. The most strict level is crazy at the moment for bind. I'm not saying it refers false positive, it returns good stuff but you are not there yet. So, this is the usage. So, you run configure then you for make and Automake, you use tool called bare which is I think like short for built ear. So, it listens what goes on and then you run CPP check on the compile commands JSON that's the compilation database and it runs the scan on all your source files. So, it's important to run your project in like a debug mode or enable everything in the configure so it gets analyzed. And the output looks like this. So, it's this is all from the tool called CPP check dash HTML report and you are lucky because the CPP check 1.90 just came out and it found a new errors because we were CPP check clean before. So, this is from the CPP check 9.90. So, it found a new errors and then if you click the individual error it looks like this. So, it annotated your source code and it chose the path through the file and it's very good. So, if you are developers and not using the CPP check I invite you to do so because we like it quite a lot. So, the next tool the Cossinell it's a program matching and transformation engine and it has something like called semantic patches. The usage is that you call the command as patch then you pass it a patch the semantic patch it can work on git repositories and so there are some other small options. So, this is and it outputs the number of files it matches according to the rules and then if there's anything that needs to be patched then it outputs the patch to the standard output or you can like patch the files in place there's an option for that too. This is a very simple semantic patch. So, what this semantic patch does that if it finds an assertion for example if you have a switch command and there's a default which always asserts like this is like custom bind assert that it insists zero. So, if the code reaches the point it crashes with assertion. So, it adds ISC unreachable which is actually just macro to attribute that the end couldn't be reached. So, it adds the ISC unreachable to all occurrences of the insist where it's not already there so that the condition behind. I'm sorry for the unicode character it's actually the normal asterisk dash but the font kept like changing that and this is more complicated example. We made a recently we made a change to the bind allocator so ISC man get cannot return null if the allocation fails the bind will crash because if you don't have enough memory then you have different kind of problems anyway with the current allocators in modern operating systems. So, this tries to catch all the occurrences where there's a statement which is you declare a statement S and then expression V. So, if V like the variable is now and then falls the statement it just deletes the whole block of the variable and there are some it's quite smart it even can understand so if V is not now and there's a block and there's an L statement you can just keep the other block which is the condition on the first block on the on the right with the S1, S2. So, it can do all kinds of transformations so it understands the C code and it transforms the code properly so it's not like a like find and replace it understands the C code and can delete the whole statement or transform them into something else. Again, if you are working with old code base like bind is and you need to do extensive refactoring this is very cool tool and you should try it. So, there are some more examples how to do stuff with Cousinel there's a web page there's we have a directory with all the semantic patches we use in bind but they are very bind specific and there's a Cousinel array.org which is even more examples in a Git repository. So, why and here comes the fun part of all it's not so funny if you have to fix all the bugs. So, who understands the memory ordering? Oh, I'm lucky I can tell you whatever I want. On a platform with weak memory consistency. Okay, me neither. Well, I'm trying. So, and what about memory barriers? Like, do you know what's that? Okay, one, two. So, and bonus question, what's lock allusion? Would anybody know? Okay, I'm not going to talk about lock allusion but it's a feature on Intel processors maybe on AMD too where you can like not block things but when they collide you roll back the whole transactions in a CPU and memory and you actually do the locking so you can avoid some locking on a hardware. Very cool but I'm sure there are some bugs hidden in there especially with Intel. So, Fred Sanitizer. This is runtime analyzer so it doesn't analyze the static code but it analyzed like running code. It requires a good test suite to run or you can run in on your production if you are brave enough. And it checks for data races. If multiple threads access the same memory location, more text ordering so if you have like multiple locks in place and they are locked in different order from different threads so there's a possibility of deadlock and other subtle errors in the inter-thread programming. The usage is again quite simple. You just add a flag to C flags or LD flags. Well, you need both and for bind we need to, I told you we are using our own implementation of RWLock so Fred Sanitizer doesn't understand that so we need to use the standard pthread RWLock here. Then you build the program, then you export some extra options. Because bind is quite complex we need to increase the history size. There's like an amount of memory the threads in there are used to keep track of the memory locations. And some other options which make it more like cleaner because for example lock path, lock path T-SEN basically means that the output is locked in the separate file and it's not mixed in the standard output. Exit code again means like usually it exits with Exit code 66 but if you are running a test suite you usually don't want to stop the test suite because it failed. So there are some like options like this then you run a test suite, bind has both unit test and system test and then there's an output like this. So as you can see there are data arrays between the two threads and there are similar locations like ISE and M-Handle on-ref but there's a data arrays and it's quite interesting data arrays because this is the on-ref function. It uses reference counting and if the reference goes to zero which is the function ISE ref count decrement so if it's not zero yet then it returns but if it's zero then it delegates resources. It should be correct right? So why is there a data arrays especially in this function? So it probably uses the reference counting and then the free function which is called here which just delegates resource and that's it. It clears the resource and then the memput is basically the ISE version of free and still there's a data arrays. It looks like innocent in a quiz right? So the fix is to either destroy to the reference handle. Why? Because the ref count API uses the standard atomics and the decrement function uses the memory ordering release and the destroy function uses the order acquire and to actually understand this this is from the documentation. So if you don't use that there's no synchronization between the threads so the destroy function, the missing destroy function made one of the threads not to synchronize with the other in decrement and then there was a data arrays because of the missing release load and like okay so who now thinks understands memory ordering? I understand memory ordering and atomics more than I understood them a year ago but I'm still like another example. There's data arrays on the PowerPC64 reported by Redhead because they do have some interesting hardware and it's caused by our own RWLock and the proposed patch again changes the memory ordering from relaxed which doesn't cause any synchronization to acquire and the compare exchange operation again to acquire. On the synthetic benchmark it works. I just look at the function and I think that basically there's an error in our RWLock implementation but we haven't seen it because the usual hardware has a strong memory consistency and it doesn't manifest so it manifests only on the PowerPC64 because it has a weak memory consistency. Again, like so but I think that there's a lesson to be learned here so to my, I like the saying don't do your own crypto and I would add don't do your own locking unless you absolutely know what you're doing. The good news is that in a new version by 916 we are actually running a benchmark now with the system library RWLock and with some other tweaks we are over one million requests per second so just dropping our own RWLock implementation increases the performance because we changed the other parts of bind. So that's good news, we might be dropping the implementation at all. Another example, there's a comment, no one else has this socket so we can free it, right? And the fix is actually to lock the socket structure because again there was a bug which no one else has the socket except when you are on FreeBSD in a VM where on a hardware with a load of cores. It was very hard to reproduce but it was reported by the FreeBSD maintainer and thank you for all the work he does by reporting the bugs to us. But it was very weird bug, we tested like with a lot of cores on Linux but it only manifests on FreeBSD in VM. If anybody can say me why then I would be happy to listen. So another thing on FreeBSD, we added to our continuous integration, we added FreeBSD and Windows in September last year, we also added OpenBSD because if you don't run in the CI on every merge request then it's like it's not happening because the developers are going to ignore the reports that came asynchronously. And there was an intermittent failure in the FreeBSD system test job and it was a crash in the LibFret system library and it was an OpenSSL exit concurrency because OpenSSL 1.1 automatically destroys the resources. But if you have multiple threats running and shutting down the resources then one of the threats were destroying the mutex is held by the OpenSSL before the OpenSSL on exit function ran. So we had to request that like there's an internal task manager in bind so in the fatal function you have to request the exclusivity of a task manager so it finishes all the other stuff before it crashes. Again, it was very weird because we were like looking why it's crashing in the system library. Here's a quote from the documentation. The other type of the bugs is the lock order inversion and this is actually the cycle at the beginning is quite short. I have some like eight mutex long bugs in bind at the moment. They are not yet fixed. But it looks like mixing the mutex in a different order from different threats. Just an example how it looks. The most simple thing you can do is to convert the locking to because there's a lot of flags in structures so we just converted this flag removed to standard atomic, dropped locking at all and it's projected by the atomics. And now you have to get the memory ordering right. So are we there yet with the threat synthesizer? Not yet. So this is the current result on the bind master. There's like ten distinct warnings from the threat synthesizer but there's something like more than 200 different paths to the warning. There's a lot of duplicates like these six errors are probably the same one and in the lock ordering there's probably also a lot of duplicates. So it's not that much bugs but we are not there yet. It doesn't crash on a regular basis. It just sometimes something happens and we don't know why. And getting this clean means that we can run the threat synthesizer in a strict mode so if there's a new warning it will stop the merge request from being merged. But as I said we are not there yet. So thank you for listening. We have seven minutes for questions. If you have any questions, note about memory ordering please. I have several. Just a very quick one. Did you also enable more compiler warnings over time? Yeah we run with WOL, WM, WM, W extra all the time. And we also have in the CI we have multiple jobs so we run with Clang, we run with GCC and we run with different optimization options because sometimes it also if you run with minus O0 sometimes it reports stuff that's not real bug but it like I don't know. There was a code that if there was a variable zero it didn't run a loop so on a higher level of optimization that got optimized out but on a lower level optimization it printed warning about something. So again like trying different optimization options from minus O0 to minus O3 is also a good option in the CI because the compilers do a different stuff. Also running the threat sanitizer with Clang and GCC various different results. So that's also thing you should do if you are running the sanitizers just do the different compilers because they created a little bit different code. Let's see your question. Just talk loud. I don't know what is our code coverage at the moment. You go back like 30 slides. I would like to ask you about C++ check. You use this tool to try to find bugs or exceptions in your code. Is it possible to use it for checking coding style? Well, Bind is written plain C so but I don't know if it's a C++ check. CPP check. Yeah, you mean this one. Is it possible to check just coding style? Some, yes. On some strictness level it also reports like variable scoping so when you can reduce the scope of the variable and some others. It's quite extensive list actually but it would be most to just show it here. This is what we use so we, this is from our CI line so we are just using these. I'm in the EMEX camp and I do my coding in EMEX so I use fly check to run CPP check and clank on my code so it automatically highlights the issues. I'm probably sure that the stuff that the young people use, all the ID so it does as well but EMEX, I like EMEX. So what's bear make? Bear make? It creates the completion database from right. Okay. Any other questions? So are you finding that CI is very expensive or slows down your team? Because I found that those are the options. So right now we are using hosted bare metal servers and we have like eight of them to run all the CI but we have some like 56 jobs at the moment. So it's getting slower and slower as we had more and more tests. We also have a Windows in the CI so it's just consumed one of the machines but the other machines are shared so if you are open BSD is ran in the QMU, free BSD had also consumed its own machine and it uses Jails. We wrote a plugin for GitLab CI to use Jails in the GitLab CI and also the open BSD that's also our own plugin. For Linux it uses the standard Docker but for the other systems we had to write custom executors for the GitLab CI. So define expensive. So if it catches back, it would cause a CVE later. Was that expensive? No, no, no. I found in some teams that it can scare people off. Like you say we are going to spend a thousand dollars a month now on just testing the code all the time. They say what can we do 100 and well yes then the team can do one feature a week. Yeah we are spending something under a hundred thousand dollars per month. Hundred thousand? No, thousand. Under thousand. Okay. One thousand. One thousand. Under one thousand a month. Under one thousand a month. It seems like a good deal. All right, thank you.
Hello, welcome everyone. I would like to present DNS developers. Okay. So, this talk is more about you don't use the tools I'm going to show you on a regular basis, it doesn't make sense at all. that but there are a couple of tools that we can working with old code base like bind is and you with all the semantic patches we use in bind but