[PATCH weston 2/8] shared: Add helpers to convert between protocol data and timespec

[Pekka Paalanen]

On Tue, 12 Dec 2017 16:29:49 +0200
Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:

> On Tue, Dec 12, 2017 at 03:09:56PM +0200, Pekka Paalanen wrote:
> > On Tue, 12 Dec 2017 14:43:11 +0200
> > Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> >   
> > > On Tue, Dec 12, 2017 at 12:09:59PM +0200, Pekka Paalanen wrote:  
> > > > On Mon,  4 Dec 2017 15:34:02 +0200
> > > > Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> > > >     
> > > > > Add helpers to safely convert between struct timespec values and
> > > > > tv_sec_hi, tv_sec_lo, tv_nsec triplets used for sending high-resolution
> > > > > timestamp data over the wayland protocol. Replace existing conversion
> > > > > code with the helper functions.
> > > > > 
> > > > > Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> > > > > ---
> > > > >  clients/presentation-shm.c |  9 +------
> > > > >  libweston/compositor.c     |  9 ++++---
> > > > >  shared/timespec-util.h     | 39 +++++++++++++++++++++++++++
> > > > >  tests/presentation-test.c  |  9 +------
> > > > >  tests/timespec-test.c      | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  5 files changed, 112 insertions(+), 20 deletions(-)    

> > > > I think it would be better to assert() that the timespec is already:
> > > > 
> > > > - normalized, because otherwise nsec might overflow in a computation,
> > > >   and    
> > > 
> > > If we make normalized input a pre-condition of timespec_to_proto, we are
> > > essentially moving the burden to the caller, which in most cases should
> > > call timespec_normalize themselves before calling timespec_to_proto. I
> > > am not sure if we should burden the callers with this and introduce the
> > > possibility of runtime errors.  
> > 
> > There would be no burden if we ensure our timestamps are always
> > normalized, which is a precondition for the timespec sub/add etc.
> > functions to operate without overflows. Therefore I'd push the
> > normalize calls to not just the callers but the sites where the
> > timestamps originate.  
> 
> I am concerned that we are making timespec_to_proto unnecessarily
> strict, and introducing more potential for runtime errors, but this is
> certainly a viable path. I will update timespec_to_proto to have this
> precondition.

I would say those are bugs deserving to be fixed, that's why assert()
exists and we use it heavily. :-)

So far we have been concentrating more on correctness and finding bugs,
rather than ensuring Weston will keep limping along no matter what.

> > > Also, I like the idea that the callers don't need to care about protocol
> > > encoding details, and just let timespec_to_proto do the conversion
> > > properly.  
> > 
> > I think there are two different "normalized" here. One is struct
> > timespec, another is what the protocol carries. Compared to struct
> > timespec, the protocol has an additional requirement of non-negativity.
> > I don't see that as leaking protocol encoding details - OTOH, we must
> > not even attempt to send values the protocol cannot carry.  
> 
> I think the difference it that passing a negative value is clearly an
> invalid value, i.e., a logic error, whereas an unnormalized valid value is
> actually a valid value with an unacceptable representation which we can
> fix.

That would be true, if our timespec helper functions would agree and
handle it. But rather than adding "fixups" on every turn, I'd prefer
ensuring we notice and fix anything that is off.

However, the major exception to that idea is any data that comes in
from "untrusted" sources like Wayland clients. That needs to be
sanitized so that clients cannot trigger compositor malfunction.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171212/5110e74b/attachment.sig>