patch for subversion (classical VM) primitiveLocalMicroseconds

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

patch for subversion (classical VM) primitiveLocalMicroseconds

stes
 

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256


Hi,

This is a patch for subversion squeakvm 4.19.6.

It relates to the primitiveLocalMicrosecondClock.

It does not affect OpenSmalltalk (I think the code in OpenSmalltalk
is different and is not affected by the following problem).

I'm not sure it is actually desired at all to implement
primitiveLocalMicrosecondClock in the 32bit case in the classical SqueakVM,
but if it is, there is an arithmetical overflow on my platform (Solaris 32bit).

I don't think it affects the classical SqueakVM Solaris 64bit by the way,
(for the 68000 type of Smalltalk images).

The patch is for the file platforms/unix/vm/sqUnixMain.c
function sqUnixUtcWithOffset().

This function is doing:

  time_t seconds= timeval.tv_sec;
  suseconds_t usec= timeval.tv_usec;
  *microSeconds= seconds * 1000000 + usec;

unfortunately the multiplication (seconds * 1000000) is triggering overflow.

The type time_t is defined for me as:
typedef long    time_t;
typedef long suseconds_t; /* signed # of microseconds */

Note that I believe that in the 32bit case this has size 4 bytes
and in the 64bit case this has size 8 bytes (for me on Solaris).

So Solaris 64bit is not affected by the multiplication error, but 32bit is.

I propose the following patch:

Index: vm/sqUnixMain.c
===================================================================
- --- vm/sqUnixMain.c (revision 3799)
+++ vm/sqUnixMain.c (working copy)
@@ -213,11 +213,13 @@
  */
 sqInt sqUnixUtcWithOffset(sqLong *microSeconds, int *offset)
 {
+  sqLong theMicroSeconds;
   struct timeval timeval;
   if (gettimeofday(&timeval, NULL) == -1) return -1;
   time_t seconds= timeval.tv_sec;
   suseconds_t usec= timeval.tv_usec;
- -  *microSeconds= seconds * 1000000 + usec;
+  theMicroSeconds = seconds;
+  *microSeconds= theMicroSeconds * 1000000 + usec;

The rationale is that the function seems to assume anyway that it is possible
to store the result in *microSeconds which is of type sqLong.

So by defining a local variable theMicroSeconds and casting seconds to that
type, and then multiplying (instead of 32bit multiply, on the 64bit),
it may work ...  at least it seems to work for me.

Maybe an alternative could be simply NOT to implement
primitiveLocalMicrosecondClock in the 32bit case ...

If primitiveLocalMicrosecondClock would return nil (primitiveFail) in the 32bit
case, it would also be a possible solution.

The primitive assumes anyway that it returns the microseconds as LargeInteger.

David Stes

PS1: in attach the patch to apply if desired

PS2: I've tested this on Solaris - all platforms are affected;
it is hard to say for me how other platforms are affected,
given their time_t definition and their sqLong definition.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJgfw4nAAoJENdFDkXGicizFVQH/3GhR/K0K9H9AyS4uBCeEIxg
lv4rCq2w9tXWeLP8eUc50sJnAsK7UVnO6FUpdFmF0qauN0414C5KmrQcIivNoEvH
kMiWduIWXfzX7VlHj56pyuYym73qOZDZowFOvnPjaVQfKcqC9yyabYrEF7THc8j3
F8bVwCy/oz1h9/Mi6VbCrjz1G+ePmowXiv5vkJFAwQDNGrc53OyhQm6Wfibdwb/8
6C2C/loPeStDf9f3Y3zhIkelp0RI4TL+h+D+REaf0kWV/TubIx648hcP4uvBhyQk
6hwD7OHHechkJLgYAni1ipGXfX03cAM2plLIgKHahypNz1mVcZhu/6SSMTQK1ik=
=gf1g
-----END PGP SIGNATURE-----

primitiveLocalMicrosecondClock.patch (840 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: patch for subversion (classical VM) primitiveLocalMicroseconds

stes
 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256


Hi,

Is it please possible to add (to subversion SVN repo) the patch for
time_t arithmetic in primitiveLocalMicrosecondClock.  See patch in attach.

Basically there is a bug with arithmetic on "signed long" (time_t).
I think it is meant to be arithmetic (multiplication) on "signed long long".

Note that for me the 64bit VM is NOT affected by this bug.

$ cc -m32 sizeoftime.c
$ ./a.out
sizeof time_t is 4
sizeof long is 4
sizeof long long is 8

$ cc -m64 sizeoftime.c
$ ./a.out
sizeof time_t is 8
sizeof long is 8
sizeof long long is 8

The problem on the 32bit VM is for me that primitiveLocalMicrosecondClock,
is trying to assign to a sqLong (long long) but it is first doing arithmetic
on the long type (which causes overflow).

David Stes

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJggaI7AAoJENdFDkXGicizjosH/jHA/28NE/jKuuo3gZQYZeLQ
Auhb/gWP8xSVDFCEJvMEvH7x+p8LhqDNWxk/F/pKgdQypnu2i/cwTHYJTLr0Knk/
mFqoNrPCk2n6Z4fFBf2YX5JyDZ9ifPO3tb+vOStypR9bf3IP5o0cmLH2JAoQ22hp
y3uZSe12JBZ6yMu3JTxrsWOZPV3rKVIeupSeA9cK7k7IL50iFOYYD+an3xqfzHVz
6ByE7VeWuTbLFdKuANndqR+xdv4ZMgmn2/EB2consfYHI5o4RRXlWhTqgvevlLNG
0ubFpKG39u26Fy+ACjD3O3e5JkiJe2U3TgvBkC3nUpxqga78ZN8IROOa+qHyEUs=
=D7S9
-----END PGP SIGNATURE-----

primitiveLocalMicrosecondClock.patch (840 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: patch for subversion (classical VM) primitiveLocalMicroseconds

David T. Lewis
In reply to this post by stes
 
Hi David,

Sorry have not had time to respond to this yet, but I'm definitely
interested in the fix.

Thanks,
Dave


On Tue, Apr 20, 2021 at 07:27:27PM +0200, [hidden email] wrote:

>
> Hi,
>
> This is a patch for subversion squeakvm 4.19.6.
>
> It relates to the primitiveLocalMicrosecondClock.
>
> It does not affect OpenSmalltalk (I think the code in OpenSmalltalk
> is different and is not affected by the following problem).
>
> I'm not sure it is actually desired at all to implement
> primitiveLocalMicrosecondClock in the 32bit case in the classical SqueakVM,
> but if it is, there is an arithmetical overflow on my platform (Solaris 32bit).
>
> I don't think it affects the classical SqueakVM Solaris 64bit by the way,
> (for the 68000 type of Smalltalk images).
>
> The patch is for the file platforms/unix/vm/sqUnixMain.c
> function sqUnixUtcWithOffset().
>
> This function is doing:
>
>   time_t seconds= timeval.tv_sec;
>   suseconds_t usec= timeval.tv_usec;
>   *microSeconds= seconds * 1000000 + usec;
>
> unfortunately the multiplication (seconds * 1000000) is triggering overflow.
>
> The type time_t is defined for me as:
> typedef long    time_t;
> typedef long suseconds_t; /* signed # of microseconds */
>
> Note that I believe that in the 32bit case this has size 4 bytes
> and in the 64bit case this has size 8 bytes (for me on Solaris).
>
> So Solaris 64bit is not affected by the multiplication error, but 32bit is.
>
> I propose the following patch:
>
> Index: vm/sqUnixMain.c
> ===================================================================
> - --- vm/sqUnixMain.c (revision 3799)
> +++ vm/sqUnixMain.c (working copy)
> @@ -213,11 +213,13 @@
>   */
>  sqInt sqUnixUtcWithOffset(sqLong *microSeconds, int *offset)
>  {
> +  sqLong theMicroSeconds;
>    struct timeval timeval;
>    if (gettimeofday(&timeval, NULL) == -1) return -1;
>    time_t seconds= timeval.tv_sec;
>    suseconds_t usec= timeval.tv_usec;
> - -  *microSeconds= seconds * 1000000 + usec;
> +  theMicroSeconds = seconds;
> +  *microSeconds= theMicroSeconds * 1000000 + usec;
>
> The rationale is that the function seems to assume anyway that it is possible
> to store the result in *microSeconds which is of type sqLong.
>
> So by defining a local variable theMicroSeconds and casting seconds to that
> type, and then multiplying (instead of 32bit multiply, on the 64bit),
> it may work ...  at least it seems to work for me.
>
> Maybe an alternative could be simply NOT to implement
> primitiveLocalMicrosecondClock in the 32bit case ...
>
> If primitiveLocalMicrosecondClock would return nil (primitiveFail) in the 32bit
> case, it would also be a possible solution.
>
> The primitive assumes anyway that it returns the microseconds as LargeInteger.
>
> David Stes
>
> PS1: in attach the patch to apply if desired
>
> PS2: I've tested this on Solaris - all platforms are affected;
> it is hard to say for me how other platforms are affected,
> given their time_t definition and their sqLong definition.
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQEcBAEBCAAGBQJgfw4nAAoJENdFDkXGicizFVQH/3GhR/K0K9H9AyS4uBCeEIxg
> lv4rCq2w9tXWeLP8eUc50sJnAsK7UVnO6FUpdFmF0qauN0414C5KmrQcIivNoEvH
> kMiWduIWXfzX7VlHj56pyuYym73qOZDZowFOvnPjaVQfKcqC9yyabYrEF7THc8j3
> F8bVwCy/oz1h9/Mi6VbCrjz1G+ePmowXiv5vkJFAwQDNGrc53OyhQm6Wfibdwb/8
> 6C2C/loPeStDf9f3Y3zhIkelp0RI4TL+h+D+REaf0kWV/TubIx648hcP4uvBhyQk
> 6hwD7OHHechkJLgYAni1ipGXfX03cAM2plLIgKHahypNz1mVcZhu/6SSMTQK1ik=
> =gf1g
> -----END PGP SIGNATURE-----

Reply | Threaded
Open this post in threaded view
|

Re: patch for subversion (classical VM) primitiveLocalMicroseconds

David T. Lewis
 
Thanks David, updated in SVN:

  ------------------------------------------------------------------------
  r3800 | lewis | 2021-04-22 17:53:06 -0400 (Thu, 22 Apr 2021) | 3 lines
 
  Integer arithmetic fix for sqUnixUtcWithOffset by David Stes
  http://lists.squeakfoundation.org/pipermail/vm-dev/2021-April/036189.html


Dave


On Thu, Apr 22, 2021 at 05:35:52PM -0400, David T. Lewis wrote:

>  
> Hi David,
>
> Sorry have not had time to respond to this yet, but I'm definitely
> interested in the fix.
>
> Thanks,
> Dave
>
>
> On Tue, Apr 20, 2021 at 07:27:27PM +0200, [hidden email] wrote:
> >
> > Hi,
> >
> > This is a patch for subversion squeakvm 4.19.6.
> >
> > It relates to the primitiveLocalMicrosecondClock.
> >
> > It does not affect OpenSmalltalk (I think the code in OpenSmalltalk
> > is different and is not affected by the following problem).
> >
> > I'm not sure it is actually desired at all to implement
> > primitiveLocalMicrosecondClock in the 32bit case in the classical SqueakVM,
> > but if it is, there is an arithmetical overflow on my platform (Solaris 32bit).
> >
> > I don't think it affects the classical SqueakVM Solaris 64bit by the way,
> > (for the 68000 type of Smalltalk images).
> >
> > The patch is for the file platforms/unix/vm/sqUnixMain.c
> > function sqUnixUtcWithOffset().
> >
> > This function is doing:
> >
> >   time_t seconds= timeval.tv_sec;
> >   suseconds_t usec= timeval.tv_usec;
> >   *microSeconds= seconds * 1000000 + usec;
> >
> > unfortunately the multiplication (seconds * 1000000) is triggering overflow.
> >
> > The type time_t is defined for me as:
> > typedef long    time_t;
> > typedef long suseconds_t; /* signed # of microseconds */
> >
> > Note that I believe that in the 32bit case this has size 4 bytes
> > and in the 64bit case this has size 8 bytes (for me on Solaris).
> >
> > So Solaris 64bit is not affected by the multiplication error, but 32bit is.
> >
> > I propose the following patch:
> >
> > Index: vm/sqUnixMain.c
> > ===================================================================
> > - --- vm/sqUnixMain.c (revision 3799)
> > +++ vm/sqUnixMain.c (working copy)
> > @@ -213,11 +213,13 @@
> >   */
> >  sqInt sqUnixUtcWithOffset(sqLong *microSeconds, int *offset)
> >  {
> > +  sqLong theMicroSeconds;
> >    struct timeval timeval;
> >    if (gettimeofday(&timeval, NULL) == -1) return -1;
> >    time_t seconds= timeval.tv_sec;
> >    suseconds_t usec= timeval.tv_usec;
> > - -  *microSeconds= seconds * 1000000 + usec;
> > +  theMicroSeconds = seconds;
> > +  *microSeconds= theMicroSeconds * 1000000 + usec;
> >
> > The rationale is that the function seems to assume anyway that it is possible
> > to store the result in *microSeconds which is of type sqLong.
> >
> > So by defining a local variable theMicroSeconds and casting seconds to that
> > type, and then multiplying (instead of 32bit multiply, on the 64bit),
> > it may work ...  at least it seems to work for me.
> >
> > Maybe an alternative could be simply NOT to implement
> > primitiveLocalMicrosecondClock in the 32bit case ...
> >
> > If primitiveLocalMicrosecondClock would return nil (primitiveFail) in the 32bit
> > case, it would also be a possible solution.
> >
> > The primitive assumes anyway that it returns the microseconds as LargeInteger.
> >
> > David Stes
> >
> > PS1: in attach the patch to apply if desired
> >
> > PS2: I've tested this on Solaris - all platforms are affected;
> > it is hard to say for me how other platforms are affected,
> > given their time_t definition and their sqLong definition.
> >
> > -----BEGIN PGP SIGNATURE-----
> > Version: GnuPG v2
> >
> > iQEcBAEBCAAGBQJgfw4nAAoJENdFDkXGicizFVQH/3GhR/K0K9H9AyS4uBCeEIxg
> > lv4rCq2w9tXWeLP8eUc50sJnAsK7UVnO6FUpdFmF0qauN0414C5KmrQcIivNoEvH
> > kMiWduIWXfzX7VlHj56pyuYym73qOZDZowFOvnPjaVQfKcqC9yyabYrEF7THc8j3
> > F8bVwCy/oz1h9/Mi6VbCrjz1G+ePmowXiv5vkJFAwQDNGrc53OyhQm6Wfibdwb/8
> > 6C2C/loPeStDf9f3Y3zhIkelp0RI4TL+h+D+REaf0kWV/TubIx648hcP4uvBhyQk
> > 6hwD7OHHechkJLgYAni1ipGXfX03cAM2plLIgKHahypNz1mVcZhu/6SSMTQK1ik=
> > =gf1g
> > -----END PGP SIGNATURE-----
>