Question about a BroString Constructor

Development
1

Hi,

I recently shot myself in the foot using the following BroString constructor:

BroString::BroString(int arg_final_NUL, byte_vec str, int arg_n)
        {
        b = str;
        n = arg_n;
        final_NUL = arg_final_NUL;
        use_free_to_delete = 0;
        }

I didn't realize that when creating a new BroString, the byte_vec I pass in doesn't get copied. I was using the byte_vec I was passing in further down, and couldn't understand why BroString wasn't acting properly. (byte_vec is defined as u_char*, not as a constant).

BroString::Set uses memcpy to copy the string. The other constructors all seem to use BroString::Set.

BroString::BroString(const u_char* str, int arg_n, int add_NUL)
      ...
       Set(str, arg_n, add_NUL);

BroString::BroString(const char* str)
      ...
        Set(str);

BroString::BroString(const string &str)
      ...
       Set(str);

The behavior is inconsistent at best. Personally, when I create a new string object, I'd expect the data to be copied, but I could see a few special cases where you might want to avoid that for performance reasons.

Any thoughts on this? Is this intended behavior? I suspect that any changes to BroString would require a lot of changes downstream.

  --Vlad

2

Agreed, it's inconsistent and error-prone. Worse, we have more such
cases in other classes as well; sometimes methods take ownership and
sometimes they don't. Unfortunately, as you say, changing these things
is hard because of the ripple effect it can have downstream, including
the potential to introduce very subtle bugs.

What we can do easily though is document these cases: just comments
about ownership would already help a lot. I'm going ahead and will add
these for the BroString ctors. Feel free to submit patches for other
cases you run across.

(Longer-term, perhaps there's somethign to do with automatic code
analysis/refactoring tools; here, for example, a tool could at least
identify all uses of that particular method, and maybe even report
what an ownership change might break down the road).

Robin