4 ms·

> Not ignore the compilation warnings – this code most likely threw a warning in the original code that was either ignored or disabled! What compiler error wou

> Not ignore the compilation warnings – this code most likely threw a warning in the original code that was either ignored or disabled! What compiler error would you expect here? Maybe not checking the return value from scanf to make sure it matches the number of parameters? Otherwise this seems like a data file error that the compiler would have no clue about.

burch45 2y ago
Undefined behavior to access the uninitialized memory. A sanitizer would have flagged that.
- jandrese 2y ago
  The compiler has no way of knowing that the memory would be undefined, not unless it somehow can verify the data file. The most I think it can do is flag the program for not checking the return value of scanf, but even that is unlikely to be true since the program probably was checking for end of file which is also in the return value. It was failing to check the number of matched parameters. This is the kind of error that is easy to miss given the semantics of scanf.
  - andrewmcwatters 2y ago
    Uninitialized variables are a really common case.
    - gmueckl 2y ago
      The pointer to the uninitialized variable is passed to scanf, which writes a value there unless it encounters an error. The compiler cannot understand this contract from the scanf declaration alone.
  - nayuki 2y ago
    > The compiler has no way of knowing that the memory would be undefined Yes it would. -fsanitize=address does a bunch of instrumentation - it allocates shadow memory to keep track of what main memory is defined, and it checks every read and write address against the shadow memory. It is a combination of compile-time instrumentation and run-time checking. And yes, it is expensive, so it should be used for debugging and not the final release. https://clang.llvm.org/docs/AddressSanitizer.html https://clang.llvm.org/docs/AddressSanitizer.html , https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=msvc-170 https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=m...
    - maccard 2y ago
      This codebase predates ASAN by the best part of a decade.
    - hoten 2y ago
      You both may be right. It could be that ASAN is not instrumenting scanf (or some other random standard lib function). Though since 2015, it certainly has been. https://github.com/google/sanitizers/issues/108 https://github.com/google/sanitizers/issues/108 The simpler policy of "don't allow unintialized locals when declared" would also have caught it with the tools available when the game was made (though a bit ham-fisted).
      - nayuki 2y ago
        The problem is that after calling scanf(), the number of variables that are defined is a variable number. For example: int x, y, z; int n = scanf("%d %d %d", &x, &y, &z); At compile time, you can make no inferences about which of x, y, and z are defined, because that depends on the returned value n. There are many ways to branch out from this. One is to insist on definite assignment - so if we cannot prove all of them are always assigned, then we can treat them as "possibly undefined" and err out. Another way is to avoid passing references and instead allow multiple returns, like Python (this is pseudocode): x, y, z = scanf("%d %d %d") In that case, if the hypothetical `scanf()` returns a tuple that is less than 3 elements or more than 3 elements, then the unpacking will fail at run time and crash exactly at that line. Another way is like Java, which insists that the return value is a scalar, so it can't do what C and Python can do. This can be painful on the programmer, of course.
        hoten 2y ago
        The idea is that ASAN would replace scanf with a function that does additional book keeping when writing to whatever arbitrary memory location the inputs dictate at runtime. It's probably what the PR resolving the issue I linked to does. Though I didn't check
        twic 2y ago
- andrewmcwatters 2y ago
  Yeah, the debugging here is great, but the actual cause is super mild.
phire 2y ago
Good point. When reading, I kind of just assumed the "use of initialised memory" warning would pick this up. But because the whole line is parsed in a single sscanf call, the compiler's static analysis is forced to assume they have now initialised. There doesn't seem to be any generic static analysis approach that can catch this bug. Though... you could make a specialised warning just for scanf that forced you to either pass in pre-initilized values or check the return result.
kristianp 2y ago
Trying g++ version 11.4, there's no warning by default if you don't check the return value of sscanf. Even `g++ -Wall -Wextra -Wunused-result` produces no warnings for a small example.

burch45 2y ago

Undefined behavior to access the uninitialized memory. A sanitizer would have flagged that.

jandrese 2y ago
The compiler has no way of knowing that the memory would be undefined, not unless it somehow can verify the data file. The most I think it can do is flag the program for not checking the return value of scanf, but even that is unlikely to be true since the program probably was checking for end of file which is also in the return value. It was failing to check the number of matched parameters. This is the kind of error that is easy to miss given the semantics of scanf.
- andrewmcwatters 2y ago
  Uninitialized variables are a really common case.
  - gmueckl 2y ago
    The pointer to the uninitialized variable is passed to scanf, which writes a value there unless it encounters an error. The compiler cannot understand this contract from the scanf declaration alone.
- nayuki 2y ago
  > The compiler has no way of knowing that the memory would be undefined Yes it would. -fsanitize=address does a bunch of instrumentation - it allocates shadow memory to keep track of what main memory is defined, and it checks every read and write address against the shadow memory. It is a combination of compile-time instrumentation and run-time checking. And yes, it is expensive, so it should be used for debugging and not the final release. https://clang.llvm.org/docs/AddressSanitizer.html https://clang.llvm.org/docs/AddressSanitizer.html , https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=msvc-170 https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=m...
  - maccard 2y ago
    This codebase predates ASAN by the best part of a decade.
  - hoten 2y ago
    You both may be right. It could be that ASAN is not instrumenting scanf (or some other random standard lib function). Though since 2015, it certainly has been. https://github.com/google/sanitizers/issues/108 https://github.com/google/sanitizers/issues/108 The simpler policy of "don't allow unintialized locals when declared" would also have caught it with the tools available when the game was made (though a bit ham-fisted).
    - nayuki 2y ago
      The problem is that after calling scanf(), the number of variables that are defined is a variable number. For example: int x, y, z; int n = scanf("%d %d %d", &x, &y, &z); At compile time, you can make no inferences about which of x, y, and z are defined, because that depends on the returned value n. There are many ways to branch out from this. One is to insist on definite assignment - so if we cannot prove all of them are always assigned, then we can treat them as "possibly undefined" and err out. Another way is to avoid passing references and instead allow multiple returns, like Python (this is pseudocode): x, y, z = scanf("%d %d %d") In that case, if the hypothetical `scanf()` returns a tuple that is less than 3 elements or more than 3 elements, then the unpacking will fail at run time and crash exactly at that line. Another way is like Java, which insists that the return value is a scalar, so it can't do what C and Python can do. This can be painful on the programmer, of course.
      - hoten 2y ago
        The idea is that ASAN would replace scanf with a function that does additional book keeping when writing to whatever arbitrary memory location the inputs dictate at runtime. It's probably what the PR resolving the issue I linked to does. Though I didn't check
      - twic 2y ago
andrewmcwatters 2y ago
Yeah, the debugging here is great, but the actual cause is super mild.

phire 2y ago

Good point. When reading, I kind of just assumed the "use of initialised memory" warning would pick this up. But because the whole line is parsed in a single sscanf call, the compiler's static analysis is forced to assume they have now initialised. There doesn't seem to be any generic static analysis approach that can catch this bug. Though... you could make a specialised warning just for scanf that forced you to either pass in pre-initilized values or check the return result.

kristianp 2y ago

Trying g++ version 11.4, there's no warning by default if you don't check the return value of sscanf. Even `g++ -Wall -Wextra -Wunused-result` produces no warnings for a small example.