1. 2
    mikegriffin avatar mikegriffin 4 years ago | link | on: How do you get started on an open source project?

    For me it’s usually fixing typos in documentation or in the code itself. If the documentation doesn’t make sense or if it’s confusing, I’ll send in some changes to make it clearer.

    Opening a pull request allows a conversation to start. The owner may not realise something is confusing because they’ve been elbows deep from the start, a new set of eyes can throw up new problems to fix.

    The main thing is using the project to find out what’s not working properly.

    1. 3
      mikegriffin avatar mikegriffin 4 years ago | link | on: different fixes for same bug

      Can anyone explain this to me? Is he saying that one of the fixes is better than the other? Or is he saying that both of them are good and both are better than the code they are fixing?

      I think he’s saying the second one, but I’m not sure.

      1. 6
        quad avatar quad 4 years ago | link

        Well, tedu hangs out here, so he can explain himself. But, here’s my interpretation:

        Both versions fix the common bug of not checking return values. In this case, ssl3_handshake_mac can return 0, and if that’s the case, then this function (ssl3_final_finish_mac) should stop and return 0.

        The first diff is better. Specifically, because it “eliminates semantic overloading of variables.” That is, in the first diff, there are only two variables: ret_md5 and ret_sha1. Unsurprisingly, these are respectively the return values for the md5 and sha1 ssl3_handshake_mac calls. The important part is when I typed “unsurprisingly.” The variables are used for one thing and assigned once (never change). They’re very easy to understand.

        Conversely, in the second diff, there is ret and sha1len. sha1len is the same as ret_sha1 in the first diff. ret, however, is both ret_md5 and then later equivalent to (ret_md5 + ret_sha1). You have the read the whole function through to understand what it is at any point because it will have two different values. The meaning of ret is semantically overloaded.

        1. 1
          mikegriffin avatar mikegriffin 4 years ago | link

          Thanks for that, that makes it a lot clearer.

        2. 5
          tedu avatar tedu 4 years ago | link

          As quad said.

          Although this function isn’t likely to go through much more development, in the second case it’s possible for someone to accidentally insert a “return ret” statement into the middle of it, reintroducing the bug. Eliminate that variable and it will be more obvious in code review that returning “ret_md5” is a bug.

          1. 3
            quad avatar quad 4 years ago | link

            I haven’t used C in anger for a long time.

            As as I recall, accumulative variables are a common pattern in a lot of classic C code. Certainly it’s come up a few times in the libressl cleanup. But, I’d never really considered the danger of a misevaluated escape creeping in.

            tedu, thanks for sharing your insight, both here and on your blog.

            My kneejerk reaction when reviewing diffs is to want code structured in a way where discrete functionality changes are grouped closed to each other. The idea being, if it has to be backed out, it happens in one place rather than all over. But, this example makes a persuasive argument for situations where that rule results in more fragile code. Food for thought!

            1. 3
              tedu avatar tedu 4 years ago | link

              Certainly, I agree minimal fixes are preferable to the CADT model but anything can be taken too far. One of the problems with OpenSSL is that it is the result of many, many years of the accretion of minimal fixes, without anyone asking if maybe the bug being fixed was representative of a larger class of bugs; or if changes could be made to prevent such bugs in the future.

              Styles and fashions change over time. There is a “unix greybeard” style of programming that goes like:

              errno = 0
foo();
bar();
baz();
bing();
bang();
boom();
if (errno) printf("oops\n");

              That’s not my preferred style, and I think it’s interesting to see that Go now effectively forbids that style of programming, making you handle each error as it occurs.

        1. 1
          mikegriffin avatar mikegriffin 4 years ago | link | on: Email Self-Defense - a guide to fighting surveillance with GnuPG

          This is a pretty good overview of how to encrypt mail, but do many people do this?

          I’ve started signing my mails with GnuPG, but I’ve yet to find anyone that encrypts their mail.

          1. 2
            mikegriffin avatar mikegriffin 4 years ago | link | on: Mini-supercomputer cluster made from 8 Parallella boards

            That’s class. Pity the boards aren’t available at the moment. This would have been brilliant for any of the parallel programming courses I did many years ago in college.

            I like, too, how he had it all designed in his head but when he saw the blinking lights thought they were too pretty to hide.

            1. 1
              zhemao avatar zhemao 4 years ago | link

              Yeah, I was also surprised that he was able to obtain eight boards.