Re: [kitten] New Version Notification for draft-kaduk-kitten-gss-loop-02.txt (fwd)

[Nico Williams <[e-mail]>]

On Fri, Jan 17, 2014 at 7:39 PM, Russ Allbery <[e-mail]> wrote:
> Nico Williams <[e-mail]> writes:
>
>> The C bindings release/delete functions generally take a pointer to a
>> variable that holds the thing being released precisely so they can set
>> that variable to the appropriate GSS_C_NO_* value.
>
> I think Martin's point is that the following is not safe:
>
>     gss_buffer_desc buf1, buf2;
>     gss_OID oid;
>     gss_name_t name;
>     OM_uint32 major, minor;
>
>     /* ... */
>
>     major = gss_display_name(&minor, name, &buf1, &oid);
>     buf2 = buf1;
>     gss_release_buffer(&minor, &buf1);
>     gss_release_buffer(&minor, &buf2);

I'm not sure that that's what he meant, but if that's what he meant
then that's fine with me.  No matter what sort of type gss_buffer_desc
might be (a struct here, but suppose it was an int or a pointer), and
considering the side-effects of gss_release_buffer(), that could not
possibly be safe.

But I think Martin meant that this is not supposed to be safe:

    gss_buffer_desc buf;
    gss_OID oid;
    gss_name_t name;
    OM_uint32 major, minor;
     /* ... */
     major = gss_display_name(&minor, name, &buf, &oid);
     gss_release_buffer(&minor, &buf); /* sets buf to {0, NULL} */
     gss_release_buffer(&minor, &buf); /* releasing {0, NULL} is safe,
like free(NULL) */

> Similar sorts of code work with other GSS-API objects.  It doesn't really
> matter whether the variable type is actually a struct or a pointer; either
> way, once you start making copies, double frees are still a risk.

Right, the type could be an integer, a pointer, a struct -- it doesn't matter.

Of course, an implementation could protect against double-release in
some cases.  But they aren't and shouldn't be required to.

What I'm saying is that the design of the C bindings' release/delete
functions is meant to set the variable whose pointer is passed in to
the zero/null/empty value.  No other interpretation is possible.  Why
ever bother taking a pointer to a variable instead of the value
otherwise?

Oh, and see the text in RFC2744, section 3.9.2:

   The minor_status parameter will always be set by a GSS-API routine,
   even if it returns a calling error or one of the generic API errors
   indicated above as fatal, although most other output parameters may
   remain unset in such cases.  However, output parameters that are
   expected to return pointers to storage allocated by a routine must
   always be set by the routine, even in the event of an error, although
   in such cases the GSS-API routine may elect to set the returned
   parameter value to NULL to indicate that no storage was actually
   allocated.  Any length field associated with such pointers (as in a
   gss_buffer_desc structure) should also be set to zero in such cases.

That only leaves the question of whether gss_release_buffer()'s
gss_buffer_t argument is an input/output parameter (it clearly has to
at least be an input parameter, but also clearly no actual output
other than {0, NULL} is intended!).

> Now, obviously, you generally should avoid doing that sort of thing in
> GSS-API code precisely because it's not safe.  But saying that it's

Or with just about *any* API.  You shouldn't close(2) the same fildes
twice, for example, nor free(3) the same pointer twice, nor munmap(2)
the same address twice, and so on.

> permissible to pass the same buffer to gss_release_buffer multiple times
> implies that this sort of thing is safe, and I'm pretty sure it isn't.

This is clearly intended to be safe:

    gss_buffer_desc buf;
    gss_OID oid;
    gss_name_t name;
    OM_uint32 major, minor;
     /* ... */
     major = gss_display_name(&minor, name, &buf, &oid);
     gss_release_buffer(&minor, &buf); /* sets buf to {0, NULL} */
     gss_release_buffer(&minor, &buf); /* releasing {0, NULL} is safe,
like free(NULL) */

Nico
--