Re: master 9f464cc: Stop using calc for ntlm time computation

Re: master 9f464cc: Stop using calc for ntlm time computation

[Thomas Fitzsimmons]

Hi Mattias,

Mattias Engdegård <mattiase@savannah.gnu.org> writes:

> branch: master
> commit 9f464ccaf9acc36b22bc292c6c572048e147d281
> Author: Mattias Engdegård <mattiase@acm.org>
> Commit: Mattias Engdegård <mattiase@acm.org>
>
>     Stop using calc for ntlm time computation
>     
>     * lisp/net/ntlm.el: Don't require calc.
>     (ntlm-compute-timestamp): Use plain arithmetic instead of calc.
>     (ntlm--time-to-timestamp): New helper function.

[...]

> -  ;; FIXME: This can likely be significantly simplified using the new
> -  ;; bignums support!

Thanks for fixing this item.

ntlm.el goes into GNU ELPA so I'd like to keep it working on older
versions of Emacs.  I usually try to keep it working at least back to
Emacs 24.1, the first version to support packages via package.el.

The new code definitely makes sense for Emacs versions that have bignum
support.  However I'd like to know before I bump the ntlm.el version
(and thus make these updates available to GNU ELPA users), are there
configurations anyone knows of where the new code will fail, but the
"calc" version would work?

I seem to remember I wrote the strange calc version to avoid integer
overflow, but I don't remember the characteristics of the system on
which I developed it; maybe it had a 32-bit word size.  Admittedly
though, I recently tried the new calculation code on Emacs 24.1, running
on an x86-64 system, and it works.

Still, if these types of calculations are known to overflow on 32-bit
Emacs builds then my inclination would be to conditionalize them.

Thanks,
Thomas

Re: master 9f464cc: Stop using calc for ntlm time computation

[Mattias Engdegård]

26 okt. 2020 kl. 22.19 skrev Thomas Fitzsimmons <fitzsim@fitzsim.org>:

> ntlm.el goes into GNU ELPA so I'd like to keep it working on older
> versions of Emacs.  I usually try to keep it working at least back to
> Emacs 24.1, the first version to support packages via package.el.

Sorry, I had no idea!

> The new code definitely makes sense for Emacs versions that have bignum
> support.  However I'd like to know before I bump the ntlm.el version
> (and thus make these updates available to GNU ELPA users), are there
> configurations anyone knows of where the new code will fail, but the
> "calc" version would work?

Yes, the new code certainly won't work with Emacs versions older than 27.1 that use 31-bit fixnums, which should be most 32-bit platforms.

> I seem to remember I wrote the strange calc version to avoid integer
> overflow, but I don't remember the characteristics of the system on
> which I developed it; maybe it had a 32-bit word size.  Admittedly
> though, I recently tried the new calculation code on Emacs 24.1, running
> on an x86-64 system, and it works.

Yes, it will work on 64-bit systems; they have 63-bit fixnums which should be enough.

> Still, if these types of calculations are known to overflow on 32-bit
> Emacs builds then my inclination would be to conditionalize them.

The change was partly made in order to remove the dependency on Calc so rather than bringing it back, I'd prefer dealing with the problem locally in ntlm.el -- either with some domain-specific cleverness or just by implementing a few basic arithmetic primitives (poor man's bignum).

However, I don't have access to any 32-bit Emacs, so I would need help for testing it.

Re: master 9f464cc: Stop using calc for ntlm time computation

[Mattias Engdegård]

26 okt. 2020 kl. 22.57 skrev Mattias Engdegård <mattiase@acm.org>:

> However, I don't have access to any 32-bit Emacs, so I would need help for testing it.

I've pushed a new implementation. Perhaps someone with a 32-bit Emacs < 27.1 would try it out. Running the test would suffice.

Re: master 9f464cc: Stop using calc for ntlm time computation

[Stefan Monnier]

Mattias Engdegård [2020-10-27 12:33:36] wrote:

> 26 okt. 2020 kl. 22.57 skrev Mattias Engdegård <mattiase@acm.org>:
>
>> However, I don't have access to any 32-bit Emacs, so I would need help for testing it.
>
> I've pushed a new implementation. Perhaps someone with a 32-bit Emacs < 27.1
> would try it out. Running the test would suffice.

Didn't get there yet.  Here's my latest attempt:

    make EMACS=/usr/bin/emacs MODULES_EMACSOPT="" test/lisp/net/ntlm-tests
    make -C test lisp/net/ntlm-tests
    make[1] : on entre dans le répertoire « /home/monnier/src/emacs/trunk/test »
    make[2] : on entre dans le répertoire « /home/monnier/src/emacs/trunk/test »
      ELC      lisp/net/ntlm-tests.elc
    
    In end of data:
    lisp/net/ntlm-tests.el:53:1:Warning: the function ‘ntlm--time-to-timestamp’ is
        not known to be defined.
      GEN      lisp/net/ntlm-tests.log
    Running 1 tests (2020-10-27 09:05:13-0400)
    Test ntlm-time-to-timestamp backtrace:
      signal(void-function (ntlm--time-to-timestamp))
      apply(signal (void-function (ntlm--time-to-timestamp)))
      (setq value-2 (apply fn-0 args-1))
      (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq form-descri
      (if (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq form-de
      (let (form-description-4) (if (unwind-protect (setq value-2 (apply f
      (let ((value-2 (quote ert-form-evaluation-aborted-3))) (let (form-de
      (let* ((fn-0 (function equal)) (args-1 (condition-case err (let ((si
      (let ((time (quote (24471 63910 412962 0)))) (let* ((fn-0 (function 
      (closure (t) nil (let ((time (quote (24471 63910 412962 0)))) (let* 
      ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
      ert-run-test(#s(ert-test :name ntlm-time-to-timestamp :documentation
      ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable)) 
      ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
      ert-run-tests-batch((not (tag :unstable)))
      ert-run-tests-batch-and-exit((not (tag :unstable)))
      eval((ert-run-tests-batch-and-exit (quote (not (tag :unstable)))))
      command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/net/ntlm-tests.el" "
      command-line()
      normal-top-level()
    Test ntlm-time-to-timestamp condition:
        (void-function ntlm--time-to-timestamp)
       FAILED  1/1  ntlm-time-to-timestamp
    
    Ran 1 tests, 0 results as expected, 1 unexpected (2020-10-27 09:05:13-0400)
    
    1 unexpected results:
       FAILED  ntlm-time-to-timestamp

Where /usr/bin/emacs is the one from Debian's testing (26.3).


        Stefan

Re: master 9f464cc: Stop using calc for ntlm time computation

[Stefan Monnier]

> Didn't get there yet.  Here's my latest attempt:

OK, I found another problem in the way I was running the tests, so
I think I finally did manage to run them correctly, which indeed finds
a real problem:

    % make EMACS="/usr/bin/emacs" EMACS_EXTRAOPT="-l /u/monnier/src/emacs/trunk/lisp/net/ntlm.el" MODULES_EMACSOPT="" test/lisp/net/ntlm-tests
    make -C test lisp/net/ntlm-tests
    make[1] : on entre dans le répertoire « /home/monnier/src/emacs/trunk/test »
    make[2] : on entre dans le répertoire « /home/monnier/src/emacs/trunk/test »
      GEN      lisp/net/ntlm-tests.log
    Running 1 tests (2020-10-27 09:42:55-0400)
    Test ntlm-time-to-timestamp backtrace:
      signal(wrong-type-argument (integerp 1.1644473632001902e+17))
      apply(signal (wrong-type-argument (integerp 1.1644473632001902e+17))
      (setq value-2 (apply fn-0 args-1))
      (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq form-descri
      (if (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq form-de
      (let (form-description-4) (if (unwind-protect (setq value-2 (apply f
      (let ((value-2 (quote ert-form-evaluation-aborted-3))) (let (form-de
      (let* ((fn-0 (function equal)) (args-1 (condition-case err (let ((si
      (let ((time (quote (24471 63910 412962 0)))) (let* ((fn-0 (function 
      (closure (t) nil (let ((time (quote (24471 63910 412962 0)))) (let* 
      ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
      ert-run-test(#s(ert-test :name ntlm-time-to-timestamp :documentation
      ert-run-or-rerun-test(#s(ert--stats :selector (not (tag :unstable)) 
      ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
      ert-run-tests-batch((not (tag :unstable)))
      ert-run-tests-batch-and-exit((not (tag :unstable)))
      eval((ert-run-tests-batch-and-exit (quote (not (tag :unstable)))))
      command-line-1(("-L" ":." "-l" "/u/monnier/src/emacs/trunk/lisp/net/
      command-line()
      normal-top-level()
    Test ntlm-time-to-timestamp condition:
        (wrong-type-argument integerp 1.1644473632001902e+17)
       FAILED  1/1  ntlm-time-to-timestamp
    
    Ran 1 tests, 0 results as expected, 1 unexpected (2020-10-27 09:42:55-0400)
    
    1 unexpected results:
       FAILED  ntlm-time-to-timestamp

The problem is that in `ntlm-tests--time-to-timestamp`, the constant
116444736000000000 is too large.


        Stefan

Re: master 9f464cc: Stop using calc for ntlm time computation

[Mattias Engdegård]

27 okt. 2020 kl. 14.06 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

>> I've pushed a new implementation. Perhaps someone with a 32-bit Emacs < 27.1
>> would try it out. Running the test would suffice.
> 
>    lisp/net/ntlm-tests.el:53:1:Warning: the function ‘ntlm--time-to-timestamp’ is
>        not known to be defined.

Sorry, I should have been more specific. You need to run an old (32-bit) Emacs but with ntlm from trunk, as if using the ELPA package.
Not quite sure how that is accomplished in practice but you figure it out.

Re: master 9f464cc: Stop using calc for ntlm time computation

[Mattias Engdegård]

27 okt. 2020 kl. 14.45 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> The problem is that in `ntlm-tests--time-to-timestamp`, the constant
> 116444736000000000 is too large.

Sorry, of course it is. Maybe you could just evaluate

  (ntlm--time-to-timestamp '(24471 63910 412962 0))

and

  (ntlm--time-to-timestamp '(397431 65535 999999 999999))

on the old and new Emacs versions and verify that the results are the same?

Needless to say, I'm very grateful for your help.

Re: master 9f464cc: Stop using calc for ntlm time computation

[Mattias Engdegård]

27 okt. 2020 kl. 15.01 skrev Mattias Engdegård <mattiase@acm.org>:

> Sorry, of course it is. Maybe you could just evaluate
> 
>  (ntlm--time-to-timestamp '(24471 63910 412962 0))
> 
> and
> 
>  (ntlm--time-to-timestamp '(397431 65535 999999 999999))
> 
> on the old and new Emacs versions and verify that the results are the same?

And before you correctly counter with 'yes but what would that prove', I meant
running ntlm-tests--time-to-timestamp in the new Emacs. Obviously.

Re: master 9f464cc: Stop using calc for ntlm time computation

[Stefan Monnier]

>>  (ntlm--time-to-timestamp '(24471 63910 412962 0))
>> 
>> and
>> 
>>  (ntlm--time-to-timestamp '(397431 65535 999999 999999))
>> 
>> on the old and new Emacs versions and verify that the results are the same?
>
> And before you correctly counter with 'yes but what would that prove', I meant
> running ntlm-tests--time-to-timestamp in the new Emacs. Obviously.

I still wouldn't know what it proves, but I'm not worried about that
anyway:

in 32bit Emacs-26.3:

    (ntlm--time-to-timestamp '(24471 63910 412962 0))
    "T\232Q\350M\254\326"
    (ntlm--time-to-timestamp '(397431 65535 999999 999999))
    "\377\x7f>a\315	;"

in 32bit `master`:

    (ntlm--time-to-timestamp '(24471 63910 412962 0))
    "T\232Q\350M\254\326"
    (ntlm-tests--time-to-timestamp '(24471 63910 412962 0))
    "T\232Q\350M\254\326"
    (ntlm--time-to-timestamp '(397431 65535 999999 999999))
    "\377\x7f>a\315	;"
    (ntlm-tests--time-to-timestamp '(397431 65535 999999 999999))
    "\377\x7f>a\315	;"

so I guess you're satisfied ;-)


        Stefan

Re: master 9f464cc: Stop using calc for ntlm time computation

[Thomas Fitzsimmons]

Mattias Engdegård <mattiase@acm.org> writes:

> 27 okt. 2020 kl. 14.06 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
>
>>> I've pushed a new implementation. Perhaps someone with a 32-bit Emacs < 27.1
>>> would try it out. Running the test would suffice.
>> 
>>    lisp/net/ntlm-tests.el:53:1:Warning: the function ‘ntlm--time-to-timestamp’ is
>>        not known to be defined.
>
> Sorry, I should have been more specific. You need to run an old (32-bit) Emacs but with ntlm from trunk, as if using the ELPA package.
> Not quite sure how that is accomplished in practice but you figure it out.

On older Emacs versions you'll need the attached too, which I haven't
pushed yet.

Thomas

diff --git a/lisp/net/ntlm.el b/lisp/net/ntlm.el
index 9401430799..1de9291017 100644
--- a/lisp/net/ntlm.el
+++ b/lisp/net/ntlm.el
@@ -152,7 +152,9 @@ ntlm--time-to-timestamp
 
 (defun ntlm-compute-timestamp ()
   "Current time as an NTLMv2 timestamp, as a unibyte string."
-  (ntlm--time-to-timestamp (time-convert nil 'list)))
+  (ntlm--time-to-timestamp (if (functionp 'time-convert)
+                               (time-convert nil 'list)
+                             (append (current-time) '(0 0)))))
 
 (defun ntlm-generate-nonce ()
   "Generate a random nonce, not to be used more than once.

Re: master 9f464cc: Stop using calc for ntlm time computation

[Mattias Engdegård]

27 okt. 2020 kl. 16.23 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> I still wouldn't know what it proves, but I'm not worried about that
> anyway: [...]
> so I guess you're satisfied ;-)

Yes, thank you! It proves that I'm not necessarily a complete dunce!

Re: master 9f464cc: Stop using calc for ntlm time computation

[Thomas Fitzsimmons]

Mattias Engdegård <mattiase@acm.org> writes:

> 27 okt. 2020 kl. 16.23 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
>
>> I still wouldn't know what it proves, but I'm not worried about that
>> anyway: [...]
>> so I guess you're satisfied ;-)
>
> Yes, thank you! It proves that I'm not necessarily a complete dunce!

Looks fine on my x86_64 Emacs 24.1 setup too.  Thanks!

Thomas