3 ms·

Rockstar fixed that bug because of this guy... Awesome!

by m-a-r-c-e-l 3y ago

rightbyte 3y ago
One could argue the bug is in scanf on Windows?
- charcircuit 3y ago
  No, the bug is not in Windows. scanf can not cache string length as it can't guarantee x, and x + offset are the same thing, nor can it guarantee the string was unmodified since the last call. Windows provides _snscanf_s if you want to keep track of the string length yourself instead of having it recompute it each time.
  - Arnavion 3y ago
    The fix would have nothing to do with caching the string length across multiple calls to sscanf. The fix would be to have sscanf not call strlen on the input string in the first place, and instead only process the input string up to the point where it satisfies the format string or the input string terminates. After all, regular scanf works fine without the length of stdin. As TFA also says: >To be fair I had no idea most sscanf implementations called strlen so I can’t blame the developer who wrote this. I would assume it just scanned byte by byte and could stop on a NULL. The author's replacement strlen does the "cache the length across calls" thing only because bolting that on top of the default strlen was easier than doing the lazy parsing thing, since the latter would've required making an actual sscanf implementation to do that from scratch.
- pjc50 3y ago
  The C standard library makes no performance guarantees. Besides, scanf is awful and should be avoided wherever possible.
  - sumtechguy 3y ago
    scanf a great way to cause a memory over/underun. Dont think so? Go read the docs for the different CRT's out there. None of them really match on what all the % items mean.
- flohofwoe 3y ago
  The problem was that the JSON parsing code was doing exceptionally stupid things in the first place (like calling scanf), which doesn't matter on small JSON files used in testing but exploded in production once that JSON file was growing to several MBytes. Just dropping in a different JSON parser library was probably enough to fix the problem on R*'s side.
  - Joker_vD 3y ago
    Why is it "exceptionally stupid"? sscanf is basically a slighlty more primitive regex engine than e.g. PCRE and I suspect it would work about as fast (if it weren't for that silly strlen() call) — and there are lexers that are basically just a loop with a match() call in it with (?P<NUMBER>?\d+(\.\d*)?)|(?P<ASSIGN>:=)|(?P<SEMI>;)|(?P<ID>[A-Za-z_][A-Za-z0-9_]*)|(?P<ARITH>[-+*/])|(?P<NEWLINE>\n)|(?P<WHITESPACE>[ \t\r]+)|(?P<MISMATCH>.) as the pattern or something like that over the input string, and that is not generally considered to be a stupid way to write a lexer. Why would sscanf be?
- Joker_vD 3y ago
  Most of the standard C library implementations, including FreeBSD's libc [0] and glibc [1] have sscanf implemented like that, by calling fscanf on a dummy FILE object (with its size populated by strlen() at every call, no caching). Of course, there are implementations whose authors thought about that and decided to do the reasonable thing instead, e.g. musl [2] and Plauger's old stdlib [3]. [0] https://github.com/lattera/freebsd/blob/master/lib/libc/stdio/vsscanf.c#L69 https://github.com/lattera/freebsd/blob/master/lib/libc/stdi... [1] https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1ec18cf65d5972e307/stdio-common/sscanf.c#L34 https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1... , https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1ec18cf65d5972e307/libio/strfile.h#L90C3-L90C31 https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1... , https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1ec18cf65d5972e307/libio/strops.c#L41 https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1... — two additional files and five times as much code compared to FreeBSD's implementation for pretty much the same functionality, wow. [2] https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsscanf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsscanf.c [3] https://github.com/wuzhouhui/c_standard_lib/blob/master/STDIO/SSCANF.C https://github.com/wuzhouhui/c_standard_lib/blob/master/STDI...
ErneX 3y ago
And paid him 10k as bounty.
- Panzer04 3y ago
  One wonders how many millions (or tens of millions) they lost from people giving up over a 10 minute loading screen...
  - ngetchell 3y ago
    Wikipedia mentions revenues for the game being $8.5 billion. It obviously didn't hurt too much!
    - jerf 3y ago
      On the other hand, if you consider the losses as a percentage based on people who gave up on the margin, you can easily derive losses relative to what they would have gotten without the bug in the tens of millions of dollars, and hundreds of millions is on the table. I can't "prove" that, because the data isn't there, but it is a completely reasonable guess. At an 8 minute load time it is reasonable to guess there were a lot of people who stopped playing and thus stopped spending because they thought their install was crashed or corrupted, and just silently wandered away. I certainly wouldn't. I've got some games that load more slowly than others but I don't think I've waited multiple minutes since the Commodore 64 era.
      - 392 3y ago
        Chiming in here as someone who used to play in competitive GTA leagues. I quit playing because of too much time spent in load screens. It was worse than just this slow loading bug: random disconnects that triggered loading again got more and more common over time, too.

Rockstar fixed that bug because of this guy... Awesome!

rightbyte 3y ago

One could argue the bug is in scanf on Windows?

charcircuit 3y ago
No, the bug is not in Windows. scanf can not cache string length as it can't guarantee x, and x + offset are the same thing, nor can it guarantee the string was unmodified since the last call. Windows provides _snscanf_s if you want to keep track of the string length yourself instead of having it recompute it each time.
- Arnavion 3y ago
  The fix would have nothing to do with caching the string length across multiple calls to sscanf. The fix would be to have sscanf not call strlen on the input string in the first place, and instead only process the input string up to the point where it satisfies the format string or the input string terminates. After all, regular scanf works fine without the length of stdin. As TFA also says: >To be fair I had no idea most sscanf implementations called strlen so I can’t blame the developer who wrote this. I would assume it just scanned byte by byte and could stop on a NULL. The author's replacement strlen does the "cache the length across calls" thing only because bolting that on top of the default strlen was easier than doing the lazy parsing thing, since the latter would've required making an actual sscanf implementation to do that from scratch.
pjc50 3y ago
The C standard library makes no performance guarantees. Besides, scanf is awful and should be avoided wherever possible.
- sumtechguy 3y ago
  scanf a great way to cause a memory over/underun. Dont think so? Go read the docs for the different CRT's out there. None of them really match on what all the % items mean.
flohofwoe 3y ago
The problem was that the JSON parsing code was doing exceptionally stupid things in the first place (like calling scanf), which doesn't matter on small JSON files used in testing but exploded in production once that JSON file was growing to several MBytes. Just dropping in a different JSON parser library was probably enough to fix the problem on R*'s side.
- Joker_vD 3y ago
  Why is it "exceptionally stupid"? sscanf is basically a slighlty more primitive regex engine than e.g. PCRE and I suspect it would work about as fast (if it weren't for that silly strlen() call) — and there are lexers that are basically just a loop with a match() call in it with (?P<NUMBER>?\d+(\.\d*)?)|(?P<ASSIGN>:=)|(?P<SEMI>;)|(?P<ID>[A-Za-z_][A-Za-z0-9_]*)|(?P<ARITH>[-+*/])|(?P<NEWLINE>\n)|(?P<WHITESPACE>[ \t\r]+)|(?P<MISMATCH>.) as the pattern or something like that over the input string, and that is not generally considered to be a stupid way to write a lexer. Why would sscanf be?
Joker_vD 3y ago
Most of the standard C library implementations, including FreeBSD's libc [0] and glibc [1] have sscanf implemented like that, by calling fscanf on a dummy FILE object (with its size populated by strlen() at every call, no caching). Of course, there are implementations whose authors thought about that and decided to do the reasonable thing instead, e.g. musl [2] and Plauger's old stdlib [3]. [0] https://github.com/lattera/freebsd/blob/master/lib/libc/stdio/vsscanf.c#L69 https://github.com/lattera/freebsd/blob/master/lib/libc/stdi... [1] https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1ec18cf65d5972e307/stdio-common/sscanf.c#L34 https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1... , https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1ec18cf65d5972e307/libio/strfile.h#L90C3-L90C31 https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1... , https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1ec18cf65d5972e307/libio/strops.c#L41 https://github.com/bminor/glibc/blob/dff8da6b3e89b986bb7f6b1... — two additional files and five times as much code compared to FreeBSD's implementation for pretty much the same functionality, wow. [2] https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsscanf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsscanf.c [3] https://github.com/wuzhouhui/c_standard_lib/blob/master/STDIO/SSCANF.C https://github.com/wuzhouhui/c_standard_lib/blob/master/STDI...

ErneX 3y ago

And paid him 10k as bounty.

Panzer04 3y ago
One wonders how many millions (or tens of millions) they lost from people giving up over a 10 minute loading screen...
- ngetchell 3y ago
  Wikipedia mentions revenues for the game being $8.5 billion. It obviously didn't hurt too much!
  - jerf 3y ago
    On the other hand, if you consider the losses as a percentage based on people who gave up on the margin, you can easily derive losses relative to what they would have gotten without the bug in the tens of millions of dollars, and hundreds of millions is on the table. I can't "prove" that, because the data isn't there, but it is a completely reasonable guess. At an 8 minute load time it is reasonable to guess there were a lot of people who stopped playing and thus stopped spending because they thought their install was crashed or corrupted, and just silently wandered away. I certainly wouldn't. I've got some games that load more slowly than others but I don't think I've waited multiple minutes since the Commodore 64 era.
    - 392 3y ago
      Chiming in here as someone who used to play in competitive GTA leagues. I quit playing because of too much time spent in load screens. It was worse than just this slow loading bug: random disconnects that triggered loading again got more and more common over time, too.