Re: Annotating libidn with Deputy

[Simon Josefsson]

Linus Nordberg <address@hidden> writes:

> Hi,
>
> I'm trying to compile libidn with Deputy[0] (part of Ivy[1]) and have a
> couple of questions for you.  But first I'd like to explain my intents
> and how I think this should be done.  Comments are welcome.

Hi.  Interesting, I haven't been following the state of the art
here... Deputy seems quite interesting, although seems to suffer from
some of the same weaknesses as Cyclone at first look.

> The goal is to be able to compile libidn with the Deputy compiler if the
> user configures --with-deputy and necessary stuff is found being
> installed on the host used for compiling.  The resulting library
> includes runtime checks inserted by the Deputy compiler and can be
> linked into an ordinary C program without any special treatment.

Cool.

> The code base of libidn should be close enough to the original code for
> ordinary C programmers to read, understand and maintain the code.  Any
> runtime penalties, execution speed and memory footprint, should be
> "small enough" for ordinary use of the library.

Yes, this is quite important.  I'm not saying the code cannot be
changed, but if is going to change, it should not be too Deputy specific
but rather a general improvement.

Possibly the simplest way to get this into the official distribution
would be to, instead of modifying code directly, prepare a patch that is
applied to the code when --with-deputy is used.  I would accept that
solution quite easily, assuming it doesn't change any official API.
Getting srcdir!=builddir builds to work is a pain, but maybe we could
document that --with-deputy only works for srcdir==builddir.

> So far, about 70% of (non generated) source code in the lib subdirectory
> has been successfully compiled with Deputy without breaking any of the
> selftests (make check).  I have admittedly used the TRUSTED keyword in
> quite a few places, resulting in less checking than possible.  This work
> should be seen as a starting point for adding more proper annotations.

If the patch is small, posting it to the list would be useful, to
understand what kind of changes are required.

> Now, questions.
>
> 1. Would it be ok to rewrite non public functions in a way that avoids
> using the same formal parameter for denoting buffer lengths as both
> input and output, i.e. replacing (size_t *len) with (size_t len_in,
> size_t *len_out)?  The API will be a bit more clunky to use, but it's
> only internal to libidn code.

I'm cautiously against this -- a size_t *len parameter is a common idiom
in C, and idioms are important for code readability.  If Deputy doesn't
support that, it seems like a significant problem with Deputy.

> 2. . In the case of public functions with an interface not well suited
> for Deputy, I'd like to rewrite them in a more Deputy friendly way,
> rename them and write a wrapper using the original function signature.
> I've suffixed the ordinary function name with `_df' so far.  Can you
> think of better naming schemes?

Adding new APIs is costly, and a limitation in a external tool isn't
justification enough to invest in this IMHO.

> 3. There are a number of functions with parameters like (char *str,
> ssize_t len) where len is the length of str unless len == -1 in which
> case str is null terminated.  Most of these are found in nfkc.c,
> f.ex. stringprep_utf8_to_ucs4().  Would it be ok to complement each of
> these with two other functions, the first one taking (char *str, size_t
> len) where len is length and the other function just taking (char *str)
> where str is null terminated?  This is more in line with other parts of
> the library, where we have functions suffixed with 'z' which are
> wrappers taking null terminated strings.

This seems indeed like a problem with the code in libidn, and improving
it would be good.  On the other hand, some of the nfkc.c code is from
glib.  There is a new NFKC implementation in gnulib that I've wanted to
use, instead of the duplicating the glib code, but I haven't made much
progress on that.  So maybe the proper fix is to replace the glib code
in nfkc.c with the gnulib NFKC implementation.

> 4. Since the source code is annotated with Deputy-specific keywords, a
> plain gcc build will need to have these keywords "removed".  This can be
> done by including a file which #define's these as empty.  Since the
> exported header files are annotated too, this file should probably be
> included by each header file and must be put in a place where the
> compiler can find them.  What would be the best way to do this?

If you use the patch approach, this wouldn't be a problem, I think.

> 5. Would it be ok to change libidn to use strncpy, strncat and friends?
> Mostly for avoiding warnings from Deputy.

Which places do you have in mind?  Some problems may be fixed in other
ways (especially in idna.c) but others is somewhat idiomatic (toutf8.c).
Some of them should probably use strdup instead of malloc+strcpy.

> 6. Most importantly, do you think that this is a good idea?  Would you
> be interested in using libidn compiled with Deputy?

I like the idea a lot in general, but the devil (as with Cyclone) is in
the details -- the 'size_t *len' problem may be a showstopper.  Maybe
the libidn code can be instrumented with TRUSTED here and the problem
reported to the Deputy people so they understand it is problem
preventing practical use?

/Simon