[Spice-devel] [PATCH 07/17] Get rid of C-style memset initializations, use C++ style aggregates
Frediano Ziglio
fziglio at redhat.com
Tue Feb 20 11:31:52 UTC 2018
> > On 20 Feb 2018, at 10:39, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> >
> > On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
> >>> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> >>>
> >>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> >>>> From: Christophe de Dinechin <dinechin at redhat.com>
> >>>>
> >>>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> >>>> ---
> >>>> src/spice-streaming-agent.cpp | 47
> >>>> ++++++++++++++++++++++++++-----------------
> >>>> 1 file changed, 28 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/src/spice-streaming-agent.cpp
> >>>> b/src/spice-streaming-agent.cpp
> >>>> index 69c27a3..1e19e43 100644
> >>>> --- a/src/spice-streaming-agent.cpp
> >>>> +++ b/src/spice-streaming-agent.cpp
> >>>> @@ -181,19 +181,24 @@ write_all(int fd, const void *buf, const size_t
> >>>> len)
> >>>> return written;
> >>>> }
> >>>>
> >>>> -static int spice_stream_send_format(int streamfd, unsigned w, unsigned
> >>>> h, unsigned c)
> >>>> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned
> >>>> h, uint8_t c)
> >>>> {
> >>>> -
> >>>> - SpiceStreamFormatMessage msg;
> >>>> - const size_t msgsize = sizeof(msg);
> >>>> - const size_t hdrsize = sizeof(msg.hdr);
> >>>> - memset(&msg, 0, msgsize);
> >>>> - msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> >>>> - msg.hdr.type = STREAM_TYPE_FORMAT;
> >>>> - msg.hdr.size = msgsize - hdrsize; /* includes only the body? */
> >>>> - msg.msg.width = w;
> >>>> - msg.msg.height = h;
> >>>> - msg.msg.codec = c;
> >>>> + const size_t msgsize = sizeof(SpiceStreamFormatMessage);
> >>>> + const size_t hdrsize = sizeof(StreamDevHeader);
> >>>> + SpiceStreamFormatMessage msg = {
> >>>> + .hdr = {
> >>>> + .protocol_version = STREAM_DEVICE_PROTOCOL,
> >>>> + .padding = 0, // Workaround GCC "not implemented" bug
> >>>> + .type = STREAM_TYPE_FORMAT,
> >>>> + .size = msgsize - hdrsize
> >>>> + },
> >>>> + .msg = {
> >>>> + .width = w,
> >>>> + .height = h,
> >>>> + .codec = c,
> >>>> + .padding1 = { }
> >>>> + }
> >>>> + };
> >>>> syslog(LOG_DEBUG, "writing format\n");
> >>>> std::lock_guard<std::mutex> stream_guard(stream_mtx);
> >>>> if (write_all(streamfd, &msg, msgsize) != msgsize) {
> >>>> @@ -204,14 +209,18 @@ static int spice_stream_send_format(int streamfd,
> >>>> unsigned w, unsigned h, unsign
> >>>>
> >>>> static int spice_stream_send_frame(int streamfd, const void *buf, const
> >>>> unsigned size)
> >>>> {
> >>>> - SpiceStreamDataMessage msg;
> >>>> - const size_t msgsize = sizeof(msg);
> >>>> ssize_t n;
> >>>> + const size_t msgsize = sizeof(SpiceStreamFormatMessage);
> >>>> + SpiceStreamDataMessage msg = {
> >>>> + .hdr = {
> >>>> + .protocol_version = STREAM_DEVICE_PROTOCOL,
> >>>> + .padding = 0, // Workaround GCC "not implemented" bug
> >>>> + .type = STREAM_TYPE_DATA,
> >>>> + .size = size /* includes only the body? */
> >>>> + },
> >>>> + .msg = {}
> >>>> + };
> >>>
> >>> So, someone should find out if we can use the designated initializers,
> >>> I suppose it depends on the compilers on all platforms we care about
> >>> supporting them?
> >>>
> >>> I wasn't able to find much useful information so far. Anyone knows in
> >>> which version of gcc it was introduced?
> >>
> >> My experience is that clang has no issue with it in any version.
> >>
> >> GCC supports it in C++ since at least 4.8.5, as tested on RHEL 7.4 with
> >> the following short example:
> >>
> >> struct Thing { int x; float y; };
> >>
> >> int foo()
> >> {
> >> Thing x = { x: 10, y:20 };
> >> Thing y = { .x = 10, .y = 20 };
> >> Thing z { 10, 20 };
> >> Thing t { .x = 10, .y = 20 };
> >> }
> >>
> >> It has, however, trouble with out-of-order initializations, with the same
> >> message in 4.8.5 as on Fedora 25 (6.4.1):
> >>
> >> glop.cpp:9:30: sorry, unimplemented: non-trivial designated initializers
> >> not supported
> >> Thing a { .y = 10, .x = 20 };
> >>
> >> The “not implemented” message is a bit scary, but the fact that the code
> >> as written has been supported as far back as 4.8.5 makes me confident
> >> that we are not in some experimental section of the compiler.
> >
> > I've found this on the matter:
> >
> > https://gcc.gnu.org/ml/gcc/2014-09/msg00207.html
> >
> > That doesn't give much confidence... Is this documented anywhere? I
> > mean I've only used Google and haven't found anything…
>
> I think it’s a low priority thing because the “fix” in source code is so easy
> (reorder the fields), and the fix complicated. According to the message you
> referenced, presently, GCC just checks that the annotations match the names,
> but otherwise ignores them and proceeds like for a POD init. If it were to
> be implemented, it would have to support a lot of corner cases mentioned in
> the message, so this makes the fix non-trivial.
>
This is not a bug but a feature. In C++20 fields have to be in the right
order so when clang will start implementing correctly the standard will
have to fix it. In C++ fields are initialized in order.
About the title aggregates are also a C style actually as we are supposed
to be C++11 is more a C style than a C++ style.
But is not this that worry me. memset(0) and aggregate initializers are
quite different, is not only a question of style. Consider this:
struct xxx {
float f;
int i;
};
xxx v { };
this initialize f and i to 0.0 and 0 respectively. On some machines
the memset does not set f to 0.0 (depends on floating point representation).
But not a big deal, we don't use floating point.
But padding worry me a bit more, consider this:
struct xxx {
uint8_t c;
uint16_t n;
};
xxx v { 1, 2 }; // or v { .c = 1, .n = 2 };
now, we know there's a byte between c and n. What's the value of this byte?
In the case of memset is 0, in the case of aggregate initializers... we don't
know! That is only the bytes occupied by the fields are initialized, not the
padding. Currently is not a big issue, there are no implicit holes and all
bytes are initialized but is this became false we would potentially have a
security issue as we are sending the full raw structure to an external
entity. At the moment the entity should be considered more secure but for
instance moving the same code on the server size would cause a security
issue. To avoid that any future structure should have no implicit padding
but this imposes an implementation detail of the protocol definition in a
project written in C (spice-protocol) just to satisfy some much higher
level style detail. It does seem to me quite easy to break this is the
future. In this respect also there are some condition which are even harder
to avoid. Consider something like
struct xxx {
uint8_t type_of_union;
uint8_t padding;
union {
uint8_t type1;
uint16_t type2;
}
};
xxx v { };
now all the bytes are covered by fields however the last byte is not
potentially initialized as C++ mandate that only first no static field
of an union is initialized.
Yes, aggregate initialization is a bit easier to read but seems that
the cost is potentially high.
> >
> > OTOH, if the extension is robust against random mistakes and typos, and
> > GCC and clang are the only compilers we care about, I think it should
> > be fine?
>
> If we agree that a message that contains “not supported” if we mess up is OK,
> I think that’s fine.
>
> However, I believe I need to beef up the comment and explain what the message
> is and what the fix is.
>
> >
> > Lukas
> >
> >> The alternatives are:
> >> - Not tagging fields at all
> >> - Tagging them “the old way”, i.e. field:value, but that has been
> >> deprecated since 2.5 and actually has the same bug as .field = value, so
> >> no benefit.
> >>
> >>
> >>>
> >>> Lukas
> >>>
> >>>> - memset(&msg, 0, msgsize);
> >>>> - msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> >>>> - msg.hdr.type = STREAM_TYPE_DATA;
> >>>> - msg.hdr.size = size; /* includes only the body? */
> >>>> std::lock_guard<std::mutex> stream_guard(stream_mtx);
> >>>> n = write_all(streamfd, &msg, msgsize);
> >>>> syslog(LOG_DEBUG,
> >>>> @@ -379,7 +388,7 @@ do_capture(int streamfd, const char *streamport,
> >>>> FILE *f_log)
> >>>>
> >>>> if (frame.stream_start) {
> >>>> unsigned width, height;
> >>>> - unsigned char codec;
> >>>> + uint8_t codec;
> >>>>
> >>>> width = frame.size.width;
> >>>> height = frame.size.height;
Maybe even better if the type is SpiceVideoCodecType here or use auto
to avoid any possible truncation.
Frediano
More information about the Spice-devel
mailing list