
Accidentally Overwriting Another Local Variable in C and C++ - ingve
https://blog.petrzemek.net/2019/12/05/accidentally-overwriting-another-local-variable-in-c-and-cpp/
======
billforsternz
As I started reading the code my very first thought was "Well obviously the
function we aren't being shown is free to wreak havoc". And it duly transpired
that thirdparty_process() was indeed wreaking havoc. Any C or C++ function
with a parameter can take the address of that parameter and use it to
overwrite random stuff on the stack frame if it wants too. In this case it's
even less surprising as the parameter is already the address of a stack frame
variable. Nothing really surprising here.

Thinking about it more you don't even need the parameter, just declare a local
variable, take its address and go crazy. With great power (it's a systems
programming language) comes great responsibility.

Wow, this is the most Hacker News comment I think I've ever posted, maybe I
need to quit Hacker News :-).

~~~
asveikau
> Thinking about it more you don't even need the parameter, just declare a
> local variable, take its address and go crazy.

If you go one layer below with some assembly, you don't even need variables.
Just take the stack pointer register (rsp on amd64) and start writing!

Or maybe no assembly needed, just cast an arbitrary hexadecimal value you
think is likely to be in range to be your stack pointer [may vary by platform,
ASLR, how many frames are on the stack, etc.] into a pointer and go to town.

------
monocasa
This kind of stuff is why I'm glad that the brogrammer fad of "ergonomic" c
library interfaces with heavily variadic function signatures has died down a
bit.

You need either a static type system, or a runtime that'll catch weird
accesses and throw immediately. A dynamic but happy to corrupt memory
interface boundary is just crazy town.

~~~
anonova
I was looking for a small image processing library (as opposed to
libMagickWand) and came across libvips [1]. I found it absolutely bizarre that
the public API in a C library was full of variadic functions to support
optional arguments. The last argument to most function calls is required to be
NULL to denote the end of the argument list, e.g.,
[https://libvips.github.io/libvips/API/current/using-
from-c.h...](https://libvips.github.io/libvips/API/current/using-
from-c.html#using-C-example) (vips_image_new_from_file, vips_invert, etc.).

[1]: [https://libvips.github.io/libvips/](https://libvips.github.io/libvips/)

~~~
zlynx
A fun one relating to 64-bit and NULL in varargs:

If your system defines NULL as 0, which is legal in C++, then NULL in the
vararg list only pushes an int-sized (4 bytes) zero, and the vararg reader
pulls a pointer-sized (8 bytes) value, which may or may NOT equal zero because
of the 4 extra bytes...

~~~
Avery3R
If using modern c++ you should use nullptr instead

~~~
monocasa
If using modern C++, you shouldn't be using varargs either.

------
Ididntdothis
This isn’t exactly surprising. With functions like printf and scanf you always
have to be super careful to pass the right data types. It may work with one OS
or compiler but not with the other. You are actually lucky if your stuff
crashes. Otherwise it will just keep going now with wrong values.

~~~
monocasa
At least for printf, there's normally a way to tell the compiler that it's a
printf style interface and get something approaching typesafety back. On gcc
this is

    
    
      extern int
      my_printf (void *my_object, const char *my_format, ...)
      __attribute__ ((format (printf, 2, 3)));
    

(Now I like rust's solution better, a sane macro system so this interface
doesn't have to be special cased by the compiler to be safe).

~~~
DerekL
C++20 is adding a type-safe alternative to printf, called std::format, though
it doesn't provide warnings at compile time.

~~~
excessive
Here's one you can use with (pretty much) any version of C++:

    
    
        __attribute__ ((format (printf, 1, 2))) 
        static inline std::string format(const char* fmt, ...) {
            va_list args;
            va_start(args, fmt);
            size_t bytes = vsnprintf(0, 0, fmt, args);
            va_end(args);
        
            va_list again;
            va_start(again, fmt);
            std::string result(bytes, '\0');
            vsnprintf(&result[0], bytes + 1, fmt, again);
            va_end(again);
            return result;
        }

------
gumby
A related problem harder for C++ to prevent is passing a pointer to a local
(say a local int) and having the callee increment the pointer. This is a form
of the sprintf bug.

C++ has references which won't suffer from this, and containers and such to
avoid sprintf()-class bugs. But if you're still writing C++ code while
thinking of C there isn't much C++ can do about it.

------
mark-r
Yawn. Yet another person discovers how easy it is to create undefined behavior
in C and C++.

------
saagarjha
I wonder if Address Sanitizer or Undefined Behavior Sanitizer would catch
this.

~~~
cozzyd
Or if one of the canary options would help (-fstack-protector-full or whatever
it's called).

~~~
saagarjha
I’m not confident that would help in this case, since the return address is
not overwritten.

------
pogsworth
my favorite flavor of this bug is: printf("%f %f %f",0,0,0);

------
tuczi
As the author said: an oldie but goodie.

------
sesuximo
Yet another reason to cover your codebase in assertions. you would have caught
this in a few seconds

~~~
monocasa
What assertation would have caught this?

~~~
sesuximo
1\. assert the integer's post condition. works best if the integer is filled
with an uninitialized pattern first.

2\. something like CRT or dmalloc to catch the leak. not perfect but a good
hint

~~~
ksherlock
So you would call thirdparty_process("whatever", &res); then assert that res
== 1? On little endian architectures, it is 1. On big endian architectures,
it's 0 which seems like a reasonable value as well.

~~~
sesuximo
depends on context, but you could either assert a range of possible values or
assert not dead

~~~
monocasa
What do you mean "not dead". It's a stack allocated variable.

~~~
sesuximo
using a sentinel value in debug mode. in this case the assertion would
probably not catch the sentinel, but you would recognize it in gdb

------
_bxg1
Reasons I hope I never have to write C(++) in the real world

~~~
unlinked_dll
As the blog post points out, in C++ this is actually remedied with variadic
templates (parameter packs). The idiomatic C++ equivalent would not have this
bug.

~~~
zlynx
Except libraries like the "third_party" example probably can't use parameter
packs just yet. They tend to have to keep compatibility with older compilers.

I used to work on a library. You wouldn't believe how long some people held
onto RHEL 5... And I had to maintain ABI compatibility with the standard
system compiler on each supported OS.

~~~
nwallin
At work we subcontracted a library. It's written in fairly idiomatic C. The
person I replaced wasn't comfortable with C so he asked the sub to write a C++
wrapper.

Lots of stuff is just a typedef if the C struct in a namespace. Classes that
do work all have private constructors and static member functions that return
a new'd pointer to the object. They also have static member functions to
deallocate the objects.

Thanks for the C++ wrapper assholes. Your C++ wrapper looks more like C than
the C library it's wrapping.

Greenfield C++ (or anything that was greenfield with C++11 in mind) is a joy
to write. Idiomatic C in C++ or idiomatic Java in C++ is terrible. The fact
that you can write so many different styles is probably the reason the
language is so ubiquitous, but it sure sucks a lot more to deal with other
people's shitty code in C++ than in many other languages.

~~~
asveikau
It seems like C programmers uncomfortable with C++ made a wrapper for C++
programmers uncomfortable with C.

I don't know why you call them assholes, they are just being themselves,
limitations and all, and providing a fairly amusing story for you to tell.

