Hi, Stefan Hagen wrote on Sat, Sep 11, 2021 at 09:00:18AM +0200:
> Is this better? No, it is no good whatsoever. * It introduces yet another pointless static buffer. * It pointlessly uses snprintf(3) where printf(3) is obviously sufficient. * It introduces error checking for operations that obviously cannot fail when done properly. * It needlessly does pointer arithmetic. So far, so bad, but the worst part is that it apparently wasn't tested on a 64-bit platform (a platform where sizeof(long) == 64, for example amd64). On such a platform, there is another bug on the very same line. The function binfmtl() has platform-dependent behaviour and does not behave as intended on a 64-bit platform and prints 32 spurious zero-digit characters, breaking the desired output alignment and making the content of the output self-contradictory on top of that. Rather than try to change binfmtl(), which is mostly pointless in the first place, let's just trivially inline the desired functionality, it's just two lines of very simple C code. OK? Ingo P.S. Usually, we would want to upstream such a bugfix. But the overall quality of this code is so abysmal that i'm not sure upstreaming anything is worth the hassle. Doing minimal cleanup of the most glaring style issues in this code would require changing 99% of it, and given how it is written throughout, my suspicion is that it is likely full of bugs, and having two bugs on this one line may not be an exception. Just look at the totally ridiculous tempstr gymnastics right below, which is incredibly stupid. Or look just a few lines above and admire the hand-rolled code handling six-byte UTF-8 characters (which are invalid according to the standard). The code is just very, very bad throughout in large numbers of respects. I'm ashamed to admit that i have occasionally been using this port myself, and after looking at the code, i'm no longer convinced i should do that. Not sure yet what to do about that. Cleaning this up looks like a project of significant size, and quite a painful one. I just looked up the author of this code: https://en.wikipedia.org/wiki/William_Poser https://billposer.org/ It looks like he is probably an eminent linguist, but he is clearly not a software developer by any stretch of the imagination. Index: Makefile =================================================================== RCS file: /cvs/ports/misc/uniutils/Makefile,v retrieving revision 1.9 diff -u -p -r1.9 Makefile --- Makefile 28 Jun 2021 21:34:19 -0000 1.9 +++ Makefile 14 Sep 2021 15:52:32 -0000 @@ -3,7 +3,7 @@ COMMENT= Unicode utilities DISTNAME= uniutils-2.27 -REVISION= 3 +REVISION= 4 CATEGORIES= misc HOMEPAGE= http://billposer.org/Software/unidesc.html Index: patches/patch-ExplicateUTF8_c =================================================================== RCS file: patches/patch-ExplicateUTF8_c diff -N patches/patch-ExplicateUTF8_c --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ patches/patch-ExplicateUTF8_c 14 Sep 2021 15:52:32 -0000 @@ -0,0 +1,30 @@ +$OpenBSD$ + +Remove %n format specifier +and stop using the platform-dependent function binfmtl(). +We want 32 bits here, not whatever sizeof(long) might be. + +Index: ExplicateUTF8.c +--- ExplicateUTF8.c.orig ++++ ExplicateUTF8.c +@@ -87,7 +87,7 @@ main(int ac, char **av){ + int UsefulBits; + unsigned char c[6]; + int i; +- UTF32 ch; ++ UTF32 ch, mask; + unsigned char *cptr; + unsigned char ShiftedByte; + char tempstr[33]; +@@ -214,7 +214,10 @@ main(int ac, char **av){ + printf("%s ",tempstr); + } + printf("\n"); +- printf("This is padded to 32 places with %d zeros: %n%s\n",(32-GotBits),&spaces,binfmtl(ch)); ++ spaces = printf("This is padded to 32 places with %d zeros: ",(32-GotBits)); ++ for (mask = 1UL << 31; mask; mask >>= 1) ++ putchar(ch & mask ? '1' : '0'); ++ putchar('\n'); + sprintf(tempstr," "); + sprintf(tempstr,"%08lX",ch); + tempstr[28] = tempstr[7];
