patch for subversion (classical VM) primitiveLocalMicroseconds

>
> 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-----

>  
> 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-----
>