[Vm-dev] patch for subversion (classical VM) primitiveLocalMicroseconds

[David T. Lewis]

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, stes at PANDORA.BE 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-----