781627 - Copy security/nss/lib/freebl/sha_fast.c to mfbt

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: copy it

https://tbpl.mozilla.org/?tree=Try&rev=ababc05e3d17

Comment 2

[Justin Lebar (not reading bugmail)]

Luke / cjones, do you mind if I review this in Waldo's absence?

Comment 3

[Justin Lebar (not reading bugmail)]

Waldo is persnickety about style in mfbt, so please forgive me for even more
nits than I would usually give.

How would you feel about making this into a proper C++ class?

>diff --git a/mfbt/Sha1.cpp b/mfbt/Sha1.cpp

I'd prefer SHA1.{cpp,h}; that matches the API.

>+#include <memory.h>

Is this necessary?

>+#include <endian.h>

Does this actually work cross-platform?

>+#include "mozilla/Sha1.h"
>+#include "Assertions.h"

mozilla/Assertions.h, if only for consistency.

The rest of this file doesn't exactly follow MFBT style, but that doesn't
really bother me, since I don't expect anybody to be hacking on this code.

>diff --git a/mfbt/Sha1.h b/mfbt/Sha1.h
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

This file needs two separate comments at the top: A short, one-liner picked up
by MXR, and a longer comment explaining how to use the interface.  See e.g.
HashFunctions.h.

>+#ifndef _SHA1_H_
>+#define _SHA1_H_

mozilla_SHA1_h_

>+#include <stdint.h>

mozilla/StandardInteger.h

>+namespace mozilla {
>+  struct SHA1Context {
>+    union {
>+      uint32_t w[16];		/* input buffer */
>+      uint8_t  b[64];
>+    } u;
>+    uint64_t size;          	/* count of hashed bytes. */
>+    unsigned H[22];		/* 5 state variables, 16 tmp values, 1 extra */
>+  };
>+
>+  void SHA1_Begin(SHA1Context *ctx);
>+  void
>+  SHA1_Update(SHA1Context *ctx, const unsigned char *dataIn, unsigned int len);

No need to wrap this; you're allowed 100 chars per line in this module.

>+  void SHA1_End(SHA1Context *ctx, unsigned char hashout[20]);
>+}

} /* namespace mozilla */

Also, please don't indent inside namespace declarations.

>+#endif

#endif /* mozilla_SHA1_h */

>diff --git a/mfbt/tests/Makefile.in b/mfbt/tests/Makefile.in
>+LIBS= $(call EXPAND_LIBNAME_PATH,mfbt,$(DEPTH)/mfbt)

I have no idea what this is for; are you confident it's right, or do you want
to ask a build peer to have a look?

>diff --git a/mfbt/tests/TestSha1.cpp b/mfbt/tests/TestSha1.cpp
>+int main() {

Newline before open brace.  (Sorry.)

>+  SHA1Context Ctx;
>+  SHA1_Begin(&Ctx);
>+  unsigned char hash[20];
>+  SHA1_Update(&Ctx, (const unsigned char*) TestV, sizeof(TestV));
>+  SHA1_End(&Ctx, hash);
>+  const unsigned char expected[20] = {
>+    0xc8, 0xf2, 0x09, 0x59, 0x4e, 0x64, 0x40, 0xaa, 0x7b, 0xf7, 0xb8, 0xe0,
>+    0xfa, 0x44, 0xb2, 0x31, 0x95, 0xad, 0x94, 0x81};
>+
>+  for (unsigned int i = 0; i < 20; ++i) {
>+    if (hash[i] != expected[i])
>+      return 1;

Brace if.

>+  }
>+  return 0;
>+}

r=me with nits addressed, although if we want to make this into a C++ class
(which would be an improvement, I think), I'd like to have another look.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Justin Lebar [:jlebar] from comment #3)
> Waldo is persnickety about style in mfbt, so please forgive me for even more
> nits than I would usually give.

thorough and fast reviews are nothing to apologize for :-)
 
> How would you feel about making this into a proper C++ class?

I think it is a good idea. I will upload a new patch in a sec.

> >+#include <memory.h>
> 
> Is this necessary?

No, string.h is a better one. Replaced.
 
> >+#include <endian.h>
> 
> Does this actually work cross-platform?

No, fixed :-(

> >diff --git a/mfbt/tests/Makefile.in b/mfbt/tests/Makefile.in
> >+LIBS= $(call EXPAND_LIBNAME_PATH,mfbt,$(DEPTH)/mfbt)
> 
> I have no idea what this is for; are you confident it's right, or do you want
> to ask a build peer to have a look?

I know we need to link mfbt library to the test (the existing tests work with just the headers). I am not completely sure if this is the canonical incantation. I will ask for feedback. On it.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: create a c++ class

Asking for khuey's feedback on the change to mfbt/tests/Makefile.in.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 652589 [details] [diff] [review]
create a c++ class

>+++ b/mfbt/SHA1.cpp
>@@ -0,0 +1,342 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include <string.h>
>+#include "mozilla/SHA1.h"
>+#include "mozilla/Assertions.h"
>+
>+// FIXME: We should probably create a more complete mfbt/Endian.h. This
>+// that any compiler that doesn't define these macros is little endian.

"This /assumes/ that"?

>diff --git a/mfbt/SHA1.h b/mfbt/SHA1.h
>new file mode 100644
>--- /dev/null
>+++ b/mfbt/SHA1.h
>@@ -0,0 +1,39 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+/* Simple class for computing SHA1. */
>+
>+/*
>+ * The constructor initializes the class, so the user only need to call the
>+ * update method on the data he wants the SHA1 of:

s/need/needs

But I'd rework this not to focus on the work the constructor does, and also to
provide a complete, working example of using the class.  (That is, give types
for buf1/buf2.)  Also, please approximate Gecko coding conventions in your code
sample -- don't user upper-case variable names.

Maybe "Example usage: {{your code}}" is sufficient.

>+#ifndef mozilla_SHA1_h_
>+#define mozilla_SHA1_h_
>+
>+#include <stdint.h>
>+namespace mozilla {
>+class SHA1Sum {
>+  union {
>+    uint32_t w[16];         /* input buffer */
>+    uint8_t  b[64];
>+  } u;
>+  uint64_t size;            /* count of hashed bytes. */
>+  unsigned H[22];           /* 5 state variables, 16 tmp values, 1 extra */
>+
>+public:
>+  SHA1Sum();
>+  void Update(const unsigned char *dataIn, unsigned int len);
>+  void End(unsigned char hashout[20]);

MFBT style is lowerCase method names (I know...), so s/Update/update/, s/End/end/.

Do you think "addBytes", "addData", or "addToHash" would be a better name for Update?  And for "End", maybe "finish"?

The signature of End() makes it impossible to pass anything but a stack buffer, right?  We should just make it take a const unsigned char*, in that case, and have a scary warning about how much memory must sit behind the pointer.

We should make 20 a public static const on the SHA1Sum class so people don't have to hardcode it.

It's an error to call End() twice, right?  Can we assert that?  We should also
assert that Update isn't called after End.  And we should indicate these
restrictions in the header file.

Anyway, this is good.  :)

Comment 7

[Justin Lebar (not reading bugmail)]

> Do you think "addBytes", "addData", or "addToHash" would be a better name for Update?

Eh, it seems like most everybody uses "update".

Python and BouncyCastle both call "end" "digest()"; maybe that's better?

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: new version

I changed End to finish. I think that is a slightly better name as it suggests that it should be used after update.

I kept the signature with char[20]. I can be used with heap allocation. A struct can have a "char sha1[20]" member for example. The only thing that would require a cast is computing a sha1 in the middle of a plain char buffer. Do you expect that to be a common case?

Comment 9

[Justin Lebar (not reading bugmail)]

> I kept the signature with char[20].

I just asked about this on IRC, because I was confused.  I thought you couldn't do

  void bar(char buf[20]);
  
  void foo() {
    char *buf;
    bar(buf);
  }

but you can, and it works fine.  In fact, you can pass a char[19] without complaint as well.

So as far as we can tell on IRC, there's no difference between char buf[20] and char* buf.

Comment 10

[Justin Lebar (not reading bugmail)]

...sorry, accidentally clicked the button.

There's no difference between |char buf[20]| and |char* buf| /as a function argument/.

So we might as well use char buf[20], since it tells us how much space you're expecting.

Comment 11

[Justin Lebar (not reading bugmail)]

> + * Using this class a function to compute the SHA1 of a buffer can be written
> + * as:

Please move "using this class" after "buffer", and put an empty line between paragraphs (here and elsewhere in the comment).

+ * void SHA1(const unsigned char* buf, unsigned size, unsigned char hash[20])

I was actually thinking we might want to make this exact signature a static method on SHA1Sum.  But, later.  :)

+ * {
+ *   SHA1Sum S;
+ *   S.update(buf, size);
+ *   S.finishnd(hash);
+ * }

finishnd

There's been a suggestion on IRC that we should use uint8_t instead of unsigned char.  I do agree we should use uint32_t for the length; we try to avoid bare int types, in general, because they have different lengths.  So if we use uint32_t, we might as well use stdint types everywhere.  So if you don't mind, maybe that can be our last change here.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 652628 [details] [diff] [review]
new version

Review of attachment 652628 [details] [diff] [review]:
-----------------------------------------------------------------

The build stuff looks ok, although I don't really understand the motivation for putting an SHA-1 hash in mfbt.

Comment 13

[Justin Lebar (not reading bugmail)]

> I don't really understand the motivation for putting an SHA-1 hash in mfbt.

It's going to be used by write-poisoning code, which needs to compute hashes after XPCOM shutdown.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/74cef2e06a35