292368 - Scriptable Hash Function API

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Doug Turner (:dougt)]

For Firefox 1.1 Software Update, there is a requirement that we be able to
create crypto hashes, specifically sha2-256, from javascript.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

the interface needs some commenting, but I think is generally what we need.


USAGE:

       var file =
Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile);

       file.initWithPath("c:\\test.txt");

       var fis =
Components.classes["@mozilla.org/network/file-input-stream;1"].createInstance(Components.interfaces.nsIFileInputStream);

       fis.init(file, -1, -1, false);
       var is = fis.QueryInterface(Components.interfaces.nsIInputStream);

       hasher.init(4); // or however you access the SHA2 value on
nsIHashFunction.
       hasher.updateFromStream(is, -1);
       dump(" -----------------------> value is: " +  hasher.finish() + "\n");

Comment 2

[Jo Hermans]

Reminds me of bug 92054 (expose MD5 in Javascript). I know that it's not the
same hash-function ...

Comment 3

[Jens Bannmann]

Comment on attachment 182189 [details] [diff] [review]
patch v.1

>RCS file: caps/idl/nsIHashFunction.idl
>+  /* Hash Algorithms (based on mozilla/security/nss/lib/cryptohi/hasht.h) */ 
>+
>+  const short MD2  = 1;
>+  const short MD5  = 2;
>+  const short SHA1 = 3;
>+  const short SHA2 = 4;

SHA2 is not a specific algorithm, but a name for a set of algorithms with
different output ranges. Therefore, I'd suggest naming your constants following
http://lxr.mozilla.org/mozilla/source/security/nss/lib/cryptohi/hasht.h#45 :

>+  const short MD2  = 1;
>+  const short MD5  = 2;
>+  const short SHA1 = 3;
>+  const short SHA256 = 4;
>+  const short SHA384 = 5;
>+  const short SHA512 = 6;

Comment 4

[Doug Turner (:dougt)]

good suggestion.  I will make this change when I update this IDL with comments 

Comment 5

[Doug Turner (:dougt)]

should we have an option to produce a ascii base64 encoded hash string? 

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

hm, you are returning the binary data in an ACString? Why not use an octet array?

although I would imagine that returning the hash as hexadecimal values (not b64
encoded) would be more useful for JS... it would match the output of md5sum, say.

how does this relate to
http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/public/nsIHash.idl ?

Comment 7

[Doug Turner (:dougt)]

maybe there should be two finish() methods, one that returns an ocet array, the
other that returns a base-64 acstring.

Darin, can comment?

The nsIHash isn't scriptable.

Comment 8

[Darin Fisher]

yeah, i like the idea of two finish functions.

i also think we should consolidate this interface with nsIHash.  we only need
one of these interfaces.

i'm still keen on using the contractid to identify the hashing function instead
of the enumeration.  i just know we are going to want to reuse this interface
with some other hash function that we will then reluctantly need to implement in
PSM when that might be inappropriate.

i'll try to review the patch this afternoon / evening.

Comment 9

[Doug Turner (:dougt)]

I will remove nsIHash in favor of nsIHashFunction.  Bigger patching coming.

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.2

This includes removing nsIHash in favor of our new scriptable nsIHashFunction.

Open issues:

* comments in the idl -- add them.
* what should the |final| method look like?
* is adding a dependency on caps evil?
* should we do the darin contract id thing?
* am I using the nsCString's right?

Comment 11

[Darin Fisher]

> * comments in the idl -- add them.

Yes, proper interface documentation is an absolute must.  We've been
too sloppy about documenting interfaces in the past.


> * what should the |final| method look like?

ACString finish(in PRBool aASCII);

That method signature is confusing.  What does the aASCII parameter
mean?  What does it mean to return non-ASCII data as an ACString?
I prefer biesi's suggestion of two finish methods that can return
the current state as either a |octet| array or as a base64-encoded
string (via ACString).  I think it might even make sense to call
this method something other than finish.  Like, maybe it is just
the current "state" of the hash.  It should be okay to ask for the
octet valued state and then ask for the base64-encoded state.


> * is adding a dependency on caps evil?

Yes, but since the current consumer of this interface is going to
be JS code (nsUpdateService.js), it doesn't really matter that much
in the short term.  Ideally, since this interface has nothing to do
with Javascript security capabilities, this interface would live 
some place else.  I suggest xpcom/ds as the concept of "hash" is
already present there (e.g., pldhash, nsHashtable, etc.).  I think
mentioning MD5, SHA2, and so on in xpcom is a bit odd, and so that's
another reason to prefer the ContractID based hash algorithm selector.
Again, why do I have to modify the interface in order to add a new
hash function?  How does an extension define a new hash function?
I might like to be able to write generic code that uses nsIHashFunction
without knowing which algorithm it is using.  Factory pattern! ;-)


> * should we do the darin contract id thing?

Yes :)


> * am I using the nsCString's right?

I'll take a look.

Comment 12

[Darin Fisher]

Comment on attachment 182506 [details] [diff] [review]
patch v.2

>Index: caps/idl/nsIHashFunction.idl

>+ * The Initial Developer of the Original Code is
>+ *

License seems broken.


>+#include "nsISupports.idl"
>+#include "nsIInputStream.idl"

nit: just forward declare nsIInputStream instead to reduce
compilation dependencies.


>Index: mailnews/extensions/smime/build/Makefile.in

> 		  pipnss \
> 		  necko \
> 		  intl \
>+		  caps \
> 		  $(NULL)

Maybe the new interface should live in pipnss, which is fine for
JS consumers.  Then, we should fix the build system to actually
make export in security/manager regardless of whether or not
--enable-crypto is specified.  That doesn't have to be this patch
since I don't see you moving consumers away from nsISignatureVerifier.


>Index: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp

>+  mDataHash = do_CreateInstance("@mozilla.org/security/hash;1", &rv);

    mDataHash = do_CreateInstance("@mozilla.org/security/hash;1?type=SHA1",
&rv);

That's nice isn't it? ;-)


>Index: mailnews/mime/src/mimemcms.cpp

>+  data->item_data = new unsigned char[hashString.Length()];
>   if (!data->item_data) return MIME_OUT_OF_MEMORY;
> 
>+  memcpy(data->item_data, hashString.get(), hashString.Length());

So, if you are okay using nsMemory::Free to release data->item_data,
then you could just replace this code with:

  data->item_data = ToNewCString(hashString);

That adds an extra null terminator to data->item_data, but that
shouldn't be a problem right?


>Index: security/manager/ssl/src/nsNSSComponent.cpp


>+NS_IMETHODIMP 
>+nsHashFunction::Init(PRUint32 algorithm)
>+{
>+  nsresult rv;
>+  mHasher = do_GetService(SIGNATURE_VERIFIER_CONTRACTID, &rv);
>+  if (NS_FAILED(rv))
>+    return rv;
>+ 
>+  if (mHashContext)
>+    HASH_Destroy(mHashContext);
>+
>+  return mHasher->HashBegin(algorithm, &mHashContext);
>+}
>+
>+NS_IMETHODIMP
>+nsHashFunction::Update(PRUint8 *data, PRUint32 len)
>+{
>+  if (!mHashContext)
>+    return NS_ERROR_NOT_INITIALIZED;
>+
>+  return mHasher->HashUpdate(mHashContext, (const char*) data, len);
>+}

Why not just use mHashContext directly?  Why go through
nsISignatureVerifier?  It seems to me that nsISignatureVerifier
should lose its hash functions in favor of this new interface,
so implementing in terms of nsISignatureVerifier seems wrong.


>+nsHashFunction::UpdateFromStream(nsIInputStream *data, PRUint32 len)
>+{
>+  if (!mHashContext)
>+    return NS_ERROR_NOT_INITIALIZED;
> 
>+  PRUint32 n;
>+  data->Available(&n);

check for errors returned from Available.


> 
>+  if (len == -1)
>+    len = n;

be sure to document that len == PR_UINT32_MAX is special.  use 
PR_UINT32_MAX instead of -1 :-)


>+  if (n == 0 || n < len)
>+    return NS_ERROR_NOT_AVAILABLE;

This error condition should be documented


>+  char* buffer = (char*) malloc(len);
>+  if (!buffer)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  nsresult rv = data->Read(buffer, len, &len);

as an optimization we could use ReadSegments to avoid
this intermediate buffer, but that's probably overkill.


>+
>+  if (NS_FAILED(rv))
>+  {
>+    free(buffer);
>+    return rv;
>+  }
>+
>+  rv = Update((unsigned char*)buffer, len);
>+  free(buffer);
>+
>+  return rv;

turn this around so you call Update if NS_SUCCEEDED(rv)
then you only have to call free(buffer) from one place.


I haven't reviewed beyond this function.  Will get to that
soonish.

Comment 13

[Doug Turner (:dougt)]

Attachment: patch v.3

This removes hashing from nsISignatureVerifier.idl, fixes up callers, addresses
most of darin's concern.  Still need to add comments to the IDL and figure out
the |final| method.

What is wrong with using an ACString again?

Comment 14

[Darin Fisher]

If you use ACString to return binary (non-ASCII) data, then it will be treated
as ISO-Latin-1 text when being converted to a JS string (i.e., it will be null
byte padded to form Unicode values).

It seems that nsIDOMWindowInternal::atob (base64 decoder) takes an AString and
returns an AString, so maybe there is convention for returning binary values as
strings.  I know that AString can handle null bytes appearing in the middle of
the string, and I seem to recall that JS strings can as well, so I guess there
is no problem with that.

Comment 15

[Brendan Eich [:brendan]]

JS has no trouble that I know of with NUL characters in strings.

/be

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 182551 [details] [diff] [review]
patch v.3

+  nsresult rv = data->Available(&n);
+  if (NS_FAILED(rv))
+    return rv;
+
+  if (len == PR_UINT32_MAX)
+    len = n;

So, why do you need to return here if Available fails? Why not just use the
passed-in length, if it's not equal to PR_UINT32_MAX?

Comment 17

[Doug Turner (:dougt)]

if available fails, why would I continue?  I don't know what state the stream is
in if available fails.  

Comment 18

[Doug Turner (:dougt)]

Attachment: patch v.4

renaming interface to nsICryptoHash.  documenting.

Comment 19

[Jens Bannmann]

Comment on attachment 182617 [details] [diff] [review]
patch v.4

>+   * use.  These values map directly onto the values defined
>+   * in mozilla/security/nss/lib/cryptohi/hasht.h.
>+   */
>+
>+    const short MD2    = 1;
>+    const short MD5    = 2;
>+    const short SHA1   = 3;
>+    const short SHA384 = 4;
>+    const short SHA512 = 5;

Either they don't map directly, or you forgot to include SHA256 = 4 :-)

Comment 20

[Doug Turner (:dougt)]

yup. i screwed up.

They should read:
    const short MD2    = 1;
    const short MD5    = 2;
    const short SHA1   = 3;
    const short SHA256 = 4;    
    const short SHA384 = 5;
    const short SHA512 = 6;

Comment 21

[Darin Fisher]

Comment on attachment 182617 [details] [diff] [review]
patch v.4

Thanks for posting the revised patch.  Here, I tear it apart.
Don't hate me!! ;-)


>Index: caps/idl/nsICryptoHash.idl

>+ * The Initial Developer of the Original Code is Doug Turner <[e-mail]>
>+ *

This is not the right template for the MPL license.  It usually
says something about the rights of the Initial Developer, etc.


>+/**
>+ * nsICryptoHash
>+ *
>+ * This interface provides crytographic hashing algorithms.
>+ */
>+
>+[scriptable, uuid(8751865e-b01d-4f33-bc13-b361dde165ed)]

nit: i think the extra newline there might throw off doxygen-like
tools.


>+interface nsICryptoHash : nsISupports
>+{
>+  /**
>+   * Hashing Algorithms.  These values are to be used by the
>+   * |init| method to indicate which hashing function to
>+   * use.  These values map directly onto the values defined
>+   * in mozilla/security/nss/lib/cryptohi/hasht.h.
>+   */

nit: 4 space indents elsewhere


>+    /**
>+     * Initalized the hashing object. This method may be
>+     * called multiple times with different algorithm types.

nit: s/Initalized/Initialize/


>+     * @throws NS_ERROR_FAILURE if an unsupported algorithm
>+     *         type is passed.

nit: maybe NS_ERROR_INVALID_ARG would be more appropriate?


>+     * @throws NS_ERROR_NOT_INITIALIZED if |init| has not been 
>+     *         called.
>+     */
>+
>+    void update([array, size_is(aLen)] in octet aData, in unsigned long aLen);

nit: again, avoid extra newlines between method and comment to 
keep doxygen-like tools happy.


>+    /**
>+     * Calculates and updates a new hash based on a given data stream.
>+     *
>+     * @param aStream an input stream to read from.
>+     *
>+     * @param aLen how much to read from the given |aStream|.  If 
>+     *        there is not enough data available in the stream as 
>+     *        |aLen|, this method will throw NS_ERROR_NOT_AVAILABLE.

nit: seems like this comments is redundant with the comment below
about the exception.  so, i would not duplicate it.  what about 
documenting the behavior of passing PR_UINT32_MAX for the aLen param?


>+    /**
>+     * Completes this hash object and produces the actual hash data.
>+     *

questions: Can I call finish again?  Do I have to call init before
I can call finish again?  Do I have to call update at least once
between init and finish?


I still disagree with putting this interface in caps.  I understand
that that isn't in the scope of this patch, so could you file a
followup bug on that issue?


>Index: caps/idl/nsISignatureVerifier.idl

> [uuid(7bdbdb36-1dd2-11b2-a44f-e303037f214d)]
> interface nsISignatureVerifier : nsISupports

You must change the UUID of any interface that is changed in a
fundamental way like this.


>Index: mailnews/base/util/nsMsgUtils.cpp


>+    rv = hasher->Update((unsigned char*) key, key_len);

nit: perhaps that cast should be to |const PRUint8*| instead.


>+    nsCString hash;
>+    rv = hasher->Finish(PR_FALSE, hash);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>+    key = (const char *) hash.get();
>     key_len = DIGEST_LENGTH;
>   }

So, this looks very bad to me.	When we exit this scope,
the destructor for |hash| will run and it will free the
memory buffer it owns.	|key| will therefore be pointing
at garbage data.  Perhaps you should just move |hash| to
function scope.

nit: |hash.get()| already returns |const char*|, so the
cast is redundant.


>+  nsCString result;
>+  rv = hasher->Init(nsICryptoHash::MD5); /* init context for 1st pass */
>+  rv = hasher->Update((unsigned char*)innerPad, 64);       /* start with inner pad */
>+  rv = hasher->Update((unsigned char*)text, text_len);     /* then text of datagram */
>+  rv = hasher->Finish(PR_FALSE, result);   /* finish up 1st pass */

nit: again, prefer |PRUint8| over |unsigned char| since that matches
the type used in the xpidl generated header file.


>+  hasher->Init(nsICryptoHash::MD5);       /* init context for 2nd pass */
>+  rv = hasher->Update((unsigned char*)outerPad, 64);    /* start with outer pad */
>+  rv = hasher->Update((unsigned char*)result.get(), 16);/* then results of 1st hash */
>+  rv = hasher->Finish(PR_FALSE, result);    /* finish up 2nd pass */
>+  memcpy(digest, result.get(), DIGEST_LENGTH);

nit: this looks like a potential ABR to me if result somehow
didn't get allocated fully.  I think you should check that
result.Length() == DIGEST_LENGTH before assigning into digest.

BTW, I'm confused... it looks like we're writing to |result|
twice here, discarding the result from the first MD5.  Or,
does Finish append to the given buffer?  If so, that should
be documented.	If so, that probably violates XPConnect since
the parameter is an out param, not an inout param.  You should
probably use two buffers and concatenate the two when you are
done.

Also, I think you should use nsCAutoString instead of nsCString
since this code would then avoid heap allocating the string
buffer.  The only difference between nsCString and nsCAutoString
is that nsCAutoString has a 64-element stack buffer at its 
disposal.



> nsresult MSGApopMD5(const char *text, PRInt32 text_len, const char *password, PRInt32 password_len, >+  
...
  rv = hasher->Update((unsigned char*) text, text_len);

nit: PRUint8


>+  rv = hasher->Finish(PR_FALSE, result);
>   NS_ENSURE_SUCCESS(rv, rv);
>-  memcpy(digest, result, DIGEST_LENGTH);
>+
>+  memcpy(digest, result.get(), DIGEST_LENGTH);

same nit about verifying the length of result.


>Index: modules/libjar/nsJAR.cpp

>+  rv = hasher->Update((unsigned char*) aInBuf, aLen);
>+  if (NS_FAILED(rv)) return rv;

nit: PRUint8


>+  *digest = ToNewCString(hashString);
>+
>   return NS_OK;

nit: verify memory allocation?

  return *digest ? NS_OK : NS_ERROR_OUT_OF_MEMORY;


>Index: netwerk/protocol/http/src/nsHttpDigestAuth.cpp

>+  nsCString hashString;

nit: use nsCAutoString here... this applies to other places
as well.


>Index: security/manager/ssl/src/nsNSSComponent.cpp


>+  mHashContext = HASH_Create((HASH_HashType) algorithm);
>+  if (mHashContext)
>+  {
>+    HASH_Begin(mHashContext);
>+    return NS_OK; 
>+  } 
>+  
>+  return NS_ERROR_FAILURE;
>+}

nit: check for errors and return failure first.  compilers
optimize for the branch not taken (i.e., the "else" clause).


>+NS_IMETHODIMP
>+nsCryptoHash::Update(PRUint8 *data, PRUint32 len)

The first parameter should be |const|.	If it is not
then the IDL needs a |const| attribute.  I should not have
to give you a non-const array pointer when you're only going
to read from it.


>+  HASH_Update(mHashContext, (const unsigned char*)data, len);

This cast should be unnecessary.


>+  PRUint32 n;
>+  nsresult rv = data->Available(&n);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  if (len == PR_UINT32_MAX)
>+    len = n;

please document this detail of the implementation.


>+  // requested, we can not fullfill the hash update.  In

(sp) fulfill


>+  char* buffer = (char*) malloc(len);
>+  if (!buffer)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  rv = data->Read(buffer, len, &len);
>+
>+  if (NS_SUCCEEDED(rv))
>+    rv = Update((unsigned char*)buffer, len);
>+
>+  free(buffer);

So, if I gave you an input stream to a 5 Mb file, you would
allocate a 5 Mb buffer, copy the file into that buffer, and
then run Update on it.	It seems to me that it would be much
better to read 4096 bytes at a time (or some chunk like that),
and then call Update several times.  Don't you agree?

You could create a stack buffer, and then run a loop where
you read some and update some until |len| has been exhausted
or an error is reached.


>+  HASH_End(mHashContext, pbuffer, &hashLen, 32);
>+  HASH_Destroy(mHashContext);

OK, so finish is called and then the object is reset to
it's pre-init state.  That definitely should be documented.


>Index: security/manager/ssl/src/nsNSSComponent.h


>+class nsCryptoHash : public nsICryptoHash
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIHASHFUNCTION

NS_DECL_NSICRYPTOHASH, right?  this probably only compiles
because your dist/include has nsIHashFunction still?

>+private:
>+  virtual ~nsCryptoHash();

nit: no reason to have a private and virtual destructor.  keep
it private, but make it non-virtual.  save a slot in the vtable.


>Index: security/manager/ssl/src/nsNSSModule.cpp

>+    NS_HASH_FUNCTION_CLASSNAME,
>+    NS_HASH_FUNCTION_CID,
>+    NS_HASH_FUNCTION_CONTRACTID,
>+    nsCryptoHashConstructor

nit: s/NS_HASH_FUNCTION/NS_CRYPTO_HASH/ ?

Comment 22

[Doug Turner (:dougt)]

The changes to mailnews/base/util/nsMsgUtils.cpp with respect to using |result|
twice is okay.  We are using one result to seed the next result.  After the call
to Update(result...), we no longer care about this string.  Passing it to
|final| again is perfectly okay.

I will fix the things you noted and post a new patch. 

Comment 23

[Doug Turner (:dougt)]

Attachment: patch v.5

fixes darin's concerns.

Comment 24

[timeless]

Comment on attachment 182687 [details] [diff] [review]
patch v.5

mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
i don't suppose i can get you to kill some tabs while you're here? :(

>+  PR_ASSERT(mHashType == nsICryptoHash::SHA1);
i really don't like PR_ASSERT. NS_ASSERTION will be fatal eventually, i don't
suppose i can get you to switch to NS_ASSERTION and proper error handling for
release builds?

>@@ -194,52 +194,52 @@ MimeMultCMS_init (MimeObject *obj)
>   data = new MimeMultCMSdata;
...
>+  data->data_hash_context = do_CreateInstance("@mozilla.org/security/hash;1", &rv);
>   if (NS_FAILED(rv)) return 0;
this leaks data, right?

> 	  if (data->decode_error)
> 		  delete data;
hrm, sure looks like it does.


>+nsCryptoHash::~nsCryptoHash()
>+{
>+  if (mHashContext)
trailing whitespace:
>+    HASH_Destroy(mHashContext);    

>+nsCryptoHash::Init(PRUint32 algorithm)
>+  mHashContext = HASH_Create((HASH_HashType) algorithm);
HASH_Create can't run out of memory?
>+  if (!mHashContext)
>+    return NS_ERROR_INVALID_ARG;


>+nsCryptoHash::UpdateFromStream(nsIInputStream *data, PRUint32 len)
>+{
NS_ENSURE_ARG_POINTER(data)! this is a public method.
>+  if (!mHashContext)
>+    return NS_ERROR_NOT_INITIALIZED;
>+  nsresult rv = data->Available(&n);

spaces around punctuation aren't consistent:
>+  if (n == 0 || n < len)
>+  while(NS_SUCCEEDED(rv) && len>0)

>+nsCryptoHash::Finish(PRBool ascii, nsACString & _retval)
>+{
isn't there an NS_ENSURE_STATE(foo, rv) or something?
>+  if (!mHashContext)
>+    return NS_ERROR_NOT_INITIALIZED;

Comment 25

[Darin Fisher]

Comment on attachment 182687 [details] [diff] [review]
patch v.5

>Index: caps/idl/nsICryptoHash.idl

>+     */
>+
>+    void init(in unsigned long aAlgorithm);

>+     */
>+
>+    void update([const, array, size_is(aLen)] in octet aData, in unsigned long 

so, you decided to ignore my nit about newlines between comments
and method declarations?


>+     *
>+     */
>+    void updateFromStream(in nsIInputStream aStream, in unsigned long aLen);

>+     */
>+    ACString finish(in PRBool aASCII);

... at least be consistent ;-)


>Index: mailnews/base/util/nsMsgUtils.cpp

>+  if (result.Length() != DIGEST_LENGTH)
>+    return NS_ERROR_FAILURE;

nit: maybe this should use

    NS_ENSURE_STATE(result.Length() == DIGEST_LENGTH);

that returns NS_ERROR_UNEXPECTED and outputs a debug warning.


r=darin with these nits plus timeless nits

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 182687 [details] [diff] [review]
patch v.5

give those nits fixed, dan can you sr.

Comment 27

[Doug Turner (:dougt)]

Attachment: patch v.6

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 182814 [details] [diff] [review]
patch v.6

>+     * @param aLen how much to read from the given |aStream|.  Passing
>+     *        PR_UINT32_MAX indicates that all data available will be used 
>+     *        to update the hash. 
>+     */
>+    void updateFromStream(in nsIInputStream aStream, in unsigned long aLen);

You ought to have a constant on this inteface for that value.
Seems like 0 would be a better value, but maybe this is a necko thing.

>+NS_IMETHODIMP
>+nsCryptoHash::UpdateFromStream(nsIInputStream *data, PRUint32 len)
>+{
>+  if (!mHashContext)
>+    return NS_ERROR_NOT_INITIALIZED;
>+
>+  PRUint32 n;
>+  nsresult rv = data->Available(&n);

Is this reliable? nsDirectoryInputStream seems to lie, for example.

sr=dveditz

Comment 29

[Darin Fisher]

It looks like whitespace in the updateFromStream loop is not so consistent. 
Also, might be good to #define that 4096 magic variable.

Dan: Yeah, PR_UINT32_MAX is the value typically used around necko to indicate an
unspecified length.  Zero does make sense in the context of this interface though.
Maybe consistency with other interfaces wins out.

Comment 30

[Doug Turner (:dougt)]

I will fix up the whitespace in the loop and #define 4096.

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 182814 [details] [diff] [review]
patch v.6

a=shaver for 1.1a2

Comment 32

[Doug Turner (:dougt)]

Checking in caps/idl/Makefile.in;
/cvsroot/mozilla/caps/idl/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvsroot/mozilla/caps/idl/nsICryptoHash.idl,v
done
Checking in caps/idl/nsICryptoHash.idl;
/cvsroot/mozilla/caps/idl/nsICryptoHash.idl,v  <--  nsICryptoHash.idl
initial revision: 1.1
done
Checking in caps/idl/nsISignatureVerifier.idl;
/cvsroot/mozilla/caps/idl/nsISignatureVerifier.idl,v  <--  nsISignatureVerifier.idl
new revision: 1.8; previous revision: 1.7
done
Checking in mailnews/base/util/nsMsgUtils.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgUtils.cpp,v  <--  nsMsgUtils.cpp
new revision: 1.104; previous revision: 1.103
done
Checking in mailnews/extensions/smime/build/Makefile.in;
/cvsroot/mozilla/mailnews/extensions/smime/build/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in mailnews/extensions/smime/src/Makefile.in;
/cvsroot/mozilla/mailnews/extensions/smime/src/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
Checking in mailnews/extensions/smime/src/nsMsgComposeSecure.cpp;
/cvsroot/mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp,v  <-- 
nsMsgComposeSecure.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in mailnews/extensions/smime/src/nsMsgComposeSecure.h;
/cvsroot/mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.h,v  <-- 
nsMsgComposeSecure.h
new revision: 1.11; previous revision: 1.10
done
Checking in mailnews/imap/src/nsImapProtocol.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapProtocol.cpp,v  <--  nsImapProtocol.cpp
new revision: 1.608; previous revision: 1.607
done
Checking in mailnews/mime/src/mimemcms.cpp;
/cvsroot/mozilla/mailnews/mime/src/mimemcms.cpp,v  <--  mimemcms.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in modules/libjar/nsJAR.cpp;
/cvsroot/mozilla/modules/libjar/nsJAR.cpp,v  <--  nsJAR.cpp
new revision: 1.116; previous revision: 1.115
done
Checking in modules/libjar/nsJAR.h;
/cvsroot/mozilla/modules/libjar/nsJAR.h,v  <--  nsJAR.h
new revision: 1.46; previous revision: 1.45
done
Checking in netwerk/build/Makefile.in;
/cvsroot/mozilla/netwerk/build/Makefile.in,v  <--  Makefile.in
new revision: 1.70; previous revision: 1.69
done
Checking in netwerk/protocol/http/src/nsHttpDigestAuth.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpDigestAuth.cpp,v  <-- 
nsHttpDigestAuth.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in netwerk/protocol/http/src/nsHttpDigestAuth.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpDigestAuth.h,v  <-- 
nsHttpDigestAuth.h
new revision: 1.5; previous revision: 1.4
done
Checking in security/manager/ssl/public/Makefile.in;
/cvsroot/mozilla/security/manager/ssl/public/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
Checking in security/manager/ssl/src/nsCMS.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsCMS.cpp,v  <--  nsCMS.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in security/manager/ssl/src/nsCMS.h;
/cvsroot/mozilla/security/manager/ssl/src/nsCMS.h,v  <--  nsCMS.h
new revision: 1.7; previous revision: 1.6
done
Checking in security/manager/ssl/src/nsNSSComponent.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v  <-- 
nsNSSComponent.cpp
new revision: 1.121; previous revision: 1.120
done
Checking in security/manager/ssl/src/nsNSSComponent.h;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.h,v  <--  nsNSSComponent.h
new revision: 1.36; previous revision: 1.35
done
Checking in security/manager/ssl/src/nsNSSModule.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSModule.cpp,v  <--  nsNSSModule.cpp
new revision: 1.37; previous revision: 1.36
done

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

is it usual to encode the hash data as base64? my impression is that it usually
is just a hexdump of the data.

Comment 34

[Peter van der Woude [:Peter6]]

this caused regression bug 296369

Comment 35

[Jens Bannmann]

Attachment: attempt to use this API from JS

I tried using this API from an extension, but couldn't get it to work. While
the empty string produced the correct hash (according to
http://en.wikipedia.org/wiki/MD5 ), e.g. "foo" produced
"693e9af84d3dfcc71e640e005bdc5e2e" where it should have been
"acbd18db4cc2f85cedef654fccc4a4d8". As nobody on IRC had an idea what might be
the reason, I'm asking here. How is this supposed to be called / what am I
doing wrong?

Comment 36

[Jens Bannmann]

Attachment: working call from JS

Okay, ignore my previous comment - here's a version that works.

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

looks like the earlier checkin didn't remove nsIHash.idl; done now,

Comment 38

[Kai Engert (:KaiE:)]

Comment on attachment 182814 [details] [diff] [review]
patch v.6

This patch introduced a call to HASH_End with fixed size 32, instead of using HASH_LENGH_MAX. See bug 383390