1273747 - Clean up nsNTLMAuthModule

Status: NEW

(Core :: Security: PSM, defect, P5)

Description

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from Bug 1271501 comment #6)
> ::: security/manager/ssl/nsNTLMAuthModule.cpp:575
> (Diff revision 1)
> >    {
> >  #ifdef IS_BIG_ENDIAN
> >      ucsDomainBuf = domain;
> >      domainPtr = ucsDomainBuf.get();
> >      domainLen = ucsDomainBuf.Length() * 2;
> > -    WriteUnicodeLE((void *) domainPtr, reinterpret_cast<const char16_t*> (domainPtr),
> > +    WriteUnicodeLE(const_cast<void*>(domainPtr),
> 
> It looks like domainPtr, userPtr, and hostPtr (as well as userUpperPtr and
> domainUpperPtr) shouldn't have been declared const (they also shouldn't all
> be declared on the same line, probably shouldn't be void*s, and

FWIW, they were probably declared const because ns(C)String.get() returns a const pointer. |void|, probably because at least one of these pointers had to take a char16ptr_t from an nsString, as well as a char* from an nsCString depending on the situation.

Regardless, all these points still stand - neither const nor void make sense here.

> WriteUnicodeLE can probably be replaced with
> mozilla::NativeEndian::swapToLittleEndianInPlace (or similar if I've
> misunderstood what's going on here), but maybe that's for another cleanup
> bug).

I believe this is correct, with the exception of https://hg.mozilla.org/mozilla-central/file/222ef20fe633/security/manager/ssl/nsNTLMAuthModule.cpp#l341, which doesn't involve only one buffer.

There are a multitude of other things that can be cleaned up about nsNTLMAuthModule (really long methods, manual memory management, goto etc) while we're here as well.

Comment 1

[:Cykesiopka]

Another thing to fix:
(David Keeler [:keeler] (use needinfo?) from Bug 1296219 comment #2)
> nsNTLMAuthModule.cpp is using both
> MOZ_LOG (via a macro called LOG) and PR_LogPrint. I think it would be best
> if it just used MOZ_LOG (perhaps with different log levels so the verbosity
> could be configured if necessary).