1

I was getting intermittently weird results with running stat() (perror(): "Bad address") on a filename that comes from a commandline argument in my project:

// mz.h
class MzImage {
private:
    /* ... */
    struct Segment {
        Dword start, stop, length;
        std::string name, type;
        Segment(Dword start, Dword stop, Dword length, const std::string &name, const std::string &type) :
            start(start), stop(stop), length(length), name(name), type(type) {}
    };

    std::vector<Segment> segs_;

public:
    MzImage(const std::string &path);
    // ...
};

// mz.cpp
MzImage::MzImage(const std::string &path) {
    cout << "path = " << path << endl;
    // ... later, path.c_str() used in stat(), turned out to be NULL
}

// mztool.cpp
int main(int argc, char* argv[]) {
    const char* mzfile = argv[1];
    cout << "Filename: " << mzfile << endl;
    try {
        MzImage mz{string(mzfile)};
        // ...
    }
    catch (Error &e) {
        cout << "Exception: " << e.what() << endl;
    }
    return 0;
}

I am noticing the filename argument is fine in main() but empty in the constructor of MzImage:

ninja@desktop:~/eaglestrike/mztools$ ./mztool a.exe
Filename: a.exe
path =

Under the debugger, the value of the string seems to disappear when entering the function:

ninja@desktop:~/eaglestrike/mztools$ gdb ./mztool
(gdb) b MzImage::MzImage
Breakpoint 1 at 0x6512: file mz.cpp, line 43.
(gdb) set args a.exe
(gdb) run
Starting program: /mnt/d/code/EagleStrike/mztools/mztool a.exe
Filename: a.exe

Breakpoint 1, MzImage::MzImage (this=0x7ffffffee100, path="a.exe") at mz.cpp:43
43      MzImage::MzImage(const std::string &path) {
(gdb) p path
$1 = "a.exe"
(gdb) n
44          cout << "path = " << path << endl;
(gdb) p path
$2 = ""

Decided to place a watch on the string, it seems to be obliterated by the vector constructor?

Breakpoint 1, MzImage::MzImage (this=0x7ffffffee100, path="a.exe") at mz.cpp:43
43      MzImage::MzImage(const std::string &path) {
(gdb) watch -l path
Watchpoint 3: -location path
(gdb) c
Continuing.

Watchpoint 3: -location path

Old value = "a.exe"
New value = <error reading variable: Cannot create a lazy string with address 0x0, and a non-zero length.>
0x000000000800bc8d in std::_Vector_base<MzImage::Segment, std::allocator<MzImage::Segment> >::_Vector_impl::_Vector_impl (this=0x7ffffffee138) at /usr/include/c++/7/bits/    stl_vector.h:89
89              : _Tp_alloc_type(), _M_start(), _M_finish(), _M_end_of_storage()
(gdb) bt
#0  0x000000000800bc8d in std::_Vector_base<MzImage::Segment, std::allocator<MzImage::Segment> >::_Vector_impl::_Vector_impl (this=0x7ffffffee138) at /usr/include/c++/7/bits/    stl_vector.h:89
#1  0x000000000800aba8 in std::_Vector_base<MzImage::Segment, std::allocator<MzImage::Segment> >::_Vector_base (this=0x7ffffffee138) at /usr/include/c++/7/bits/stl_vector.h:127
#2  0x000000000800a228 in std::vector<MzImage::Segment, std::allocator<MzImage::Segment> >::vector (this=0x7ffffffee138) at /usr/include/c++/7/bits/stl_vector.h:263
#3  0x0000000008006547 in MzImage::MzImage (this=0x7ffffffee100, path=<error reading variable: Cannot create a lazy string with address 0x0, and a non-zero length.>) at     mz.cpp:43
#4  0x0000000008005d19 in main (argc=2, argv=0x7ffffffee268) at mztool.cpp:22

Any idea on what might be happenning? This is g++ 7.5.0 on Ubuntu 18.04 running under WSL.

neuviemeporte
  • 6,310
  • 10
  • 49
  • 78
  • Add `-Wshadow` to your compiler options and then fix the problems with shadowed variables. (`stop`, `start`, `length`, `name`, `type`)... Ex `Segment(Dword _start, Dword _stop, ...` and then `start(_start), stop(_stop), ...` – David C. Rankin Jun 14 '21 at 22:38
  • 1
    If I make a couple trivial changes to this program so that it actually compiles for me (adding `using Dword = unsigned int;`, replacing `Exception&` with `const std::exception&`, putting in some includes), I can't reproduce the described behavior. It prints `Filename: foo` and `path = foo` when I give `foo` as a command-line argument. Can you edit this to make it a reproducible example? – Nathan Pierson Jun 14 '21 at 23:29
  • 1
    Create a [mcve] – eerorika Jun 14 '21 at 23:33
  • Can't reproduce: https://godbolt.org/z/s6shsrK9W – Joseph Sible-Reinstate Monica Jun 15 '21 at 00:14
  • Sorry for not providing a minimal example, I had problems reproducing it on the side myself (see the answer), thanks for taking the time to try and reproduce it. – neuviemeporte Jun 15 '21 at 17:27

2 Answers2

1

Sorry for not providing enough information, I wasn't able to reproduce the problem at will myself because it showed up intermittently. The problem turned out to be caused by a Makefile screwup, of all things:

all: mztool

objs = mztool.o mz.o util.o
CC = g++
CXXFLAGS = -std=c++14 -g

mztool: $(objs)

I have introduced changes in mz.h, adding members to MzImage, so the object size increased. After running make, mz.o was rebuilt, but mztool.o wasn't, so I ended up linking two object files with different ideas of the MzImage object. The code of main() in mztool.o created the object mz on the stack, but the size was too small. Later, code from mz.o ran the vector constructor which has overwritten the string argument on the stack. Ouch. Also, the issue would disappear whenever I ran make clean or introduced changes to mztool.cpp, because then mztool.o would get rebuilt with the updated MzImage layout.

The solution was to introduce proper dependencies in the Makefile, so that (among others) mztool.o is rebuilt whenever mz.h changes:

mztool.o: mz.cpp mz.h
mz.o: mz.cpp mz.h util.cpp util.h
neuviemeporte
  • 6,310
  • 10
  • 49
  • 78
0

The primary problem is your constructor parameter names shadow your member variable names. You need to provide different names for your parameters, e.g.

Segment(Dword _start, Dword _stop, Dword _length, 
    const std::string &_name, const std::string &_type) :
start(_start), stop(_stop), length(_length), 
name(_name), type(_type) {}

Unless you have a separate #define, for Error, you need to replace the exception type with const std::exception.

With those changes, your code compiles just fine (except for the Unused argc -- which you can remedy with (void)argc; placed after return 0;)

The lesson is to always compile with full warnings enabled and to add -Wshadow explicitly to flag any shadowed variables (can be very helpful in circumstances just like this). A good set of warning for your situation can be:

g++ -Wall -Wextra -pedantic -Wshadow -std=c++17

That will catch 99% of the issues.

The additional issue is one disclosed in your comment:

// ... later, path.c_str() used in stat(), turned out to be NULL

where .c_str() is taken on the temporary object from MzImage mz{string(mzfile)};. Which is likely the reason for the problem you are experiencing. (Thanks to Nathan Pierson) std::string::c_str() and temporaries contains a full explanation.

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
  • 3
    Doesn't the use of the member initialization list mean that shadowing isn't occurring? Couldn't the problem be related to taking the `.c_str()` of a temporary? – Nathan Pierson Jun 14 '21 at 23:18
  • Shadowing is still occurring, e.g. `warning: declaration of ‘type’ shadows a member of ‘MzImage::Segment’ [-Wshadow]` -- that doesn't cause the overwriting (warning) but it is a mess. You are likely correct on the temporary. – David C. Rankin Jun 14 '21 at 23:22
  • 1
    @DavidC.Rankin I have not downvoted you, but 1) shadowing would not have caused wiping variable values in a different class, in a different stack frame, and 2) there is no reason why you couldn't take a `c_str()` of a temporary (within its lifetime), as the answer you are linking to helpfully explains. – neuviemeporte Jun 15 '21 at 17:11
  • 1
    Yes, with only a comment describing the use of `.c_str()` and the use of the word "later", it was impossible to know whether the lifetime of the temporary had expired. Turns out it was a make file issue anyway. – David C. Rankin Jun 15 '21 at 17:57