[AGENT++] MT-safety of get_printable() (was: SNMP++ advocacy)

[maom_onet at poczta.onet.pl]

> Well, I think that there are two options:
> - call get_printable() just after an object has been created, to
> pre-format its buffer and seal the state
> - use locks to synchronize access

Unfortunately, I did not notice that both method just won't work. Only address classes behave in the first way, and locks will not help if SNMP++ calls get_printable() internally (which does happen, although I have not analyzed it carefully).

What I write below only concerns objects that are not modified but possibly multiple threads are calling get_printable().

A comment to SnmpSyntax::get_printable() promises:

* @note The returned string is valid as long as the object is not
*       modified.

which is, unfortunately, not true in a number of cases - see below.

I would say that there are two problems:
- assuring "weak MT-safety," that is, allowing get_printable() to return invalid data but at least guarantee no buffer "overreads"
- assuring "strong MT-safety": data will be always right

1. Address classes

Address classes use the following approach:

if (addr_changed) format_output(); return output_buffer;

and format_output() calls sprintf() functions. If sprintf() functions modify the buffer (i.e., they do not repeat exactly the sequence of bytes written to the buffer to a previous call), then I think currently address classes guarantee weak MT-safety. That is, when two threads are calling get_printable(), the buffer content returned in one thread may be not exactly the sequence just written by this thread. Example: assuming that sprintf("%d.%d",...) is internally implemented by two separate calls to some _internal_print_int(), the sequence written by a first thread may be
123.456
but the buffer's content may temporarily become 123\0 (when other thread is writting to the buffer). However, if sprintf() functions write byte-by-byte, the issue disappears.

A possible approach would be:

if (addr_changed)
{
   char buf[LARGE_ENOUGH];
   format_output(buf);
   strcpy(output_buffer, buf);
   addr_changed = false;
}
return output_buffer;

There is some overhead but this guarantees "strong MT-safety".

2. SMI classes

This is more complex. Most of simple classes (e.g., integer) seem to be ok for "weak MT-safety" but a "Strong" one would require a similar approach to the above (with the flag or without).

OctetStr::get_printable*() functions contain memory re-allocation, and this is completely insecure. Even if two threads are just reading. Looks like there is no other simple approach than locking.

Oid::get_printable() is even worse as it seems to always re-allocate buffer (even if there is no change to the object), which violates significantly the statement about buffer validity. It seems that locking will also be difficult to avoid here. Additional problems stem from fact that get_printable() has a number of forms for this class, and all of them reuse the buffer.

3. Remark

This function seems to be a more serious problem that I have suspected before. Especially that it is used internally by SNMP++ (and maybe by Agent++). I think that a good solution would be to:

- add a thread-safe version of get_printable():

char* SnmpSyntax::get_printable(char* buf, int* buflen)

that would guarantee MT-safety and return NULL(0) when buflen is too small ('buflen' could be set on return to a minimum length needed).

- migrate all existing internal code to this new version
- add a compilation option that would allow to completely remove the current (insecure) one

Yes, I realize that this is uneasy. Thus, this is currently a proposal for discussion rather than a request for feature.

4. It is also possible that SNMP++/Agent++ internally only use these functions on a single thread, which would alleviate the issue. Which means that it may never happen they will be called for 'const' objects supplied by arguments, for example, and also that they do not call these functions after handing it to another thread. In such a case, for my SNMP++.NET to guarantee MT-safery (on read, for const objects), I think I should avoid calling get_printable() and provide own implementations. However, in such a case, the description (and thus requirements) for SnmpSyntax::get_printable() should be changed and warn that it is not MT-safe, even on (MT-) read.

Best regards,

Marek