Re: [PATCH] Fix variable-sized c strings

[Justus Winter]

Quoting Samuel Thibault (2014-02-22 16:54:28)
> Justus Winter, le Fri 21 Feb 2014 23:12:03 +0100, a écrit :
> > Previously, the terminating zero of variable-sized c strings was only
> > included when copying the message if the length of the string was not
> > a multiple of four.  mig_strncpy returns the length of the string
> > excluding the terminating zero.  Fix this by properly accounting for
> > the byte for the terminating zero in the array length.
> 
> Do you know in which case this code is actually hit?  I have a hard time
> reviewing the change without knowing what the generated code around
> looks like :)

Yes.  Consider the client-side stub generated for task_set_name:

[...]
        const mach_msg_type_t nameType = {
                /* msgt_name = */               MACH_MSG_TYPE_STRING_C,
[...]
        };
        InP->nameType = nameType;
        InP->nameType.msgt_number = __mig_strncpy(InP->name, name, 64);
        msgh_size = 28 + ((InP->nameType.msgt_number + 3) & ~3);
[...]

So InP->nameType.msgt_number carries the length of the variable
string.  The comment right above the code generating the mig_strncpy
call says:

/* Copy variable-size C string with mig_strncpy.  Save the string
   length (+ 1 for trailing 0) in the argument`s count field.  */

So it was intended to include the terminating 0.  However, mig_strncpy
as implemented in the glibc does not include the terminating zero in
the length it returns (and mig_strncpy in the kernel was a void
function).  So for mig stubs running in userspace, the terminating
zero was only transmitted if the padding in the message size
computation caused some bytes to be copied as padding for the "name"
array.

So this change adds

    if (InP->nameType.msgt_number < 64)
        InP->nameType.msgt_number += 1;

to include the terminating zero when sending a message (a similar
change should be done for the receiving stubs to ensure proper
termination).

There is one caveat, if a string of more than 64 bytes is provided,
the string in the message wont be zero-terminated.  Reading the
"server writers guide", it seems like it was intended to include the 0
in the message, so it's probably wise to always zero-terminate the
string:

    if (InP->nameType.msgt_number < 64)
        InP->nameType.msgt_number += 1;
    else
        InP->name[64 - 1] = '\0';

That however silently truncates the string (then again, mig_strncpy
does that already).

Justus

> 
> > * server.c (WritePackArgValue): Account for the terminating zero in
> > the array length.
> > * user.c (WritePackArgValue): Likewise.
> > ---
> >  server.c | 5 +++++
> >  user.c   | 5 +++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/server.c b/server.c
> > index 129cec3..a3368f6 100644
> > --- a/server.c
> > +++ b/server.c
> > @@ -912,6 +912,11 @@ WritePackArgValue(FILE *file, const argument_t *arg)
> >               arg->argMsgField,
> >               arg->argVarName,
> >               it->itNumber);
> > +         fprintf(file,
> > +             "    if (OutP->%s < %d) OutP->%s += 1;\n",
> > +             arg->argCount->argMsgField,
> > +             it->itNumber,
> > +             arg->argCount->argMsgField);
> >       }
> >       else {
> >           argument_t *count = arg->argCount;
> > diff --git a/user.c b/user.c
> > index 37f53d2..f4a6cd5 100644
> > --- a/user.c
> > +++ b/user.c
> > @@ -411,6 +411,11 @@ WritePackArgValue(FILE *file, const argument_t *arg)
> >               arg->argMsgField,
> >               arg->argVarName,
> >               it->itNumber);
> > +         fprintf(file,
> > +             "    if (InP->%s < %d) InP->%s += 1;\n",
> > +             arg->argCount->argMsgField,
> > +             it->itNumber,
> > +             arg->argCount->argMsgField);
> >       }
> >       else {
> >  
> > -- 
> > 1.8.5.2
> > 
> 
> -- 
> Samuel
> <k> faut en profiter, aujourd'hui, les blagues bidon sont à 100 dollars
>  -+- #sos-bourse -+-