[Linux-NTFS-Dev] how to fix bugs

[Andras E. <cc...@fr...>]

[sorry for the long post]

i'm thinking about contributing some effort to your project;
a good first step can be getting familiar with already existing=20
code, thus i picked mkntfs.c and started reading it

there seem to be a few bugs in the code, so while reading it
i might as well try to fix them, but i don't know what kind
of fixes would you prefer, so i selected a trivial function
with a trivial bug, and written three fixes -- please let me
know which one would you prefer, and flame me a bit, so that
my future fixes can get through more easily (if you want them
at all :O)

the function in question is stoucs(); it gets the size of
the output buffer in bytes (as maxlen), but in fact it can
write (maxlen-2)*2+2 bytes into it (e.g. into an output
buffer of 10 bytes it writes up to 18 bytes):

/**
 * stoucs - convert ASCII string to unicode-character string
 * @dest: points to buffer to receive the converted string
 * @src:  points to string to convert
 * @maxlen: size of @dest buffer in bytes
 *
 * Return the number of characters written to @dest, not including the
 * terminating null unicode character.
 */
int stoucs(uchar_t *dest, const char *src, int maxlen)
{
  char c;
  int i;

  /* Need two bytes for null terminator. */
  maxlen -=3D 2;
  for (i =3D 0; i < maxlen; i++) {
    c =3D src[i];
    if (!c)
      break;
    dest[i] =3D cpu_to_le16(c);
  }
  dest[i] =3D cpu_to_le16('\0');
  return i;
}

the first fix is the minimalistic one:

--- mkntfs.c.orig       2004-02-25 15:29:28.000000000 +0100
+++ mkntfs.c    2004-02-25 16:23:38.000000000 +0100
@@ -569,7 +569,7 @@

        /* Need two bytes for null terminator. */
        maxlen -=3D 2;
-       for (i =3D 0; i < maxlen; i++) {
+  for (i =3D 0; i < maxlen/2; i++) {
                c =3D src[i];
                if (!c)
                        break;

the second fix preserves the original method, with a
little cleanup:

int stoucs( uchar_t * dst, char const * src, int size )
{
  int const uni_size =3D size / sizeof( *dst ) ;
  int cp ;

  for ( cp =3D 0 ; src[ cp ] && cp < uni_size - 1 ; ++cp )
      dst[ cp ] =3D cpu_to_le16( src[ cp ] ) ;

  dst[ cp ] =3D cpu_to_le16( '0' ) ;

  return cp ;
}

the third fix is an imho more c-ish complete rewrite
(+ assert + comments):

#include <assert.h>

unsigned stoucs( uchar_t * dst, char const * src, unsigned const size )
{
  char const * const begin =3D src ;
  unsigned uni_size =3D size / sizeof( *dst ) ;

  assert( uni_size ) ;

  // while there's both input and output left
  while ( *src && --uni_size )
      *dst++ =3D cpu_to_le16( *src++ ) ; // ascii -> unicode

  // make sure the output is terminated even when there wasn't
  // enough space to convert the whole input
  *dst =3D cpu_to_le16( '0' ) ;

  return src - begin ;
}

i've also created a revised version of the comment describing
the function:

/**
 * stoucs - convert ASCIIZ string to unicode string
 *
 * @param dst [out] points the buffer to receive the unicode string
 * @param src [in] points the char string to be converted
 * @param size [in] size of the @dst buffer in bytes
 *
 * @returns the number of chars converted, excluding the
 * terminating null unicode character.
 *
 * @par constraint: @dst must be able to hold at least the
 * terminating null [@size >=3D sizeof( uchar_t )]
 * @see ucstos
 */


br,
andras

ps: i've compiled the above changes, but haven't tested
them, so please don't use them yet