enums , ints, and authz

[Paul Burba <pa...@softlanding.com>]

Hello All,

Note: This is *not* an iSeries specific question, so please don't let your 
eyes glaze over at the start :-)

While working on our ebcdic port of subversion to the IBM iSeries we 
experienced an unusual(?) problem:

When compiling a C program on the iSeries there is an option to set the 
size of enums to 1 byte, 2 bytes, 4 bytes, the ANSI-standard for ints (4 
bytes), or the smallest number of bytes that can contain the enum.  The 
last option is the default and what we used.  We had no problems when 
building subversion with no optimization, but when we built with full 
optimization, authz started behaving inconsistently, denying access and 
granting access with seemingly no relation the authz file being used. 
After much painful debugging I was able to track the problem down to 
libsvn/repos/authz.c's authz_access_is_granted() and 
authz_access_is_determined():

static svn_boolean_t
authz_access_is_granted (svn_repos_authz_access_t allow,
                         svn_repos_authz_access_t deny,
                         svn_repos_authz_access_t required)
{
  svn_repos_authz_access_t stripped_req =
    required & (svn_authz_read | svn_authz_write);

  if ((deny & required) == svn_authz_none)
    return TRUE;
  else if ((allow & required) == stripped_req)
    return TRUE;
  else
    return FALSE;
}

static svn_boolean_t
authz_access_is_determined (svn_repos_authz_access_t allow,
                            svn_repos_authz_access_t deny,
                            svn_repos_authz_access_t required)
{
  if ((deny & required) || (allow & required))
    return TRUE;
  else
    return FALSE;
}

The optimized code didn't perform the bitwise operations in these 
functions correctly when the enums were stored in the smallest possible 
byte size.  What is interesting is that we had no problems in 1.2 using 
the same authz file.  In 1.2 the functionality of 
authz_access_is_granted() and authz_access_is_determined() was in 
mod_dav_authz.c and was directly in the functions parse_authz_lines() and 
parse_authz_section(), e.g.:

static int parse_authz_lines(svn_config_t *cfg,
                             const char *repos_name, const char 
*repos_path,
                             const char *user,
                             int required_access, int *granted_access,
                             apr_pool_t *pool)
{
    const char *qualified_repos_path;
    struct parse_authz_baton baton = { 0 };

    baton.pool = pool;
    baton.config = cfg;
    baton.user = user;

    /* First try repos specific */
    qualified_repos_path = apr_pstrcat(pool, repos_name, ":", repos_path,
                                       NULL);
    svn_config_enumerate(cfg, qualified_repos_path,
                         parse_authz_line, &baton);
    *granted_access = !(baton.deny & required_access)
                      || (baton.allow & required_access);

    if ((baton.deny & required_access)
        || (baton.allow & required_access))
        return TRUE;

    svn_config_enumerate(cfg, repos_path,
                         parse_authz_line, &baton);
    *granted_access = !(baton.deny & required_access)
                      || (baton.allow & required_access);

    return (baton.deny & required_access)
           || (baton.allow & required_access);
} 

Back to our 1.3 problem, I found that by casting the arguments in authz.c 
to ints the problem went away, e.g.:

static svn_boolean_t
authz_access_is_determined (svn_repos_authz_access_t allow,
                            svn_repos_authz_access_t deny,
                            svn_repos_authz_access_t required)
{
  if (((int)deny & (int)required) || ((int)allow & (int)required))
    return TRUE;
  else
    return FALSE;
}

Removing these casts, we rebuilt our fully optimized code using the option 
to always make enums 4 bytes and the authz problems went away.  My best 
guess is that 1.2 had no problems because the AUTHZ_SVN_* enums were 
always implicitly cast to an int when used.  In 1.3 however, with the 
creation of authz_access_is_granted() and authz_access_is_determined(), 
ints (authz_lookup_baton->allow, deny, required_access) were passed to 
functions that take 1-byte enums and something went awry. 

Anyway, this gets (finally!) to my ultimate questions:

1) Could this problem afflict other platforms that have compile options to 
dictate enum size?  (e.g. gcc's -fshort-enums option)

2) In C is it always safe to pass an int argument to a function that takes 
an enum argument?  The ever-trusty Harbinson and Steele C Reference Manual 
*seems* to indicate this is fine for compilers that strictly implement the 
standard, but then warns to not do it:

"...enumerated data types in Standard C...act as little more than slightly 
readable ways to name integer constants.  As a matter of style, we suggest 
that programmers treat enumerated types as different from integers and not 
mix them in integer expressions without using casts.  In fact, some UNIX C 
compilers implement a weakly typed form of enumerations in which some 
conversions between enumerated types and integers are not permitted 
without casts."

Any thoughts from the C gurus out there are appreciated.

Thanks,

Paul B.

_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs. 
_____________________________________________________________________________

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: enums , ints, and authz

[Branko Čibej <br...@xbc.nu>]

Paul Burba wrote:
> Hello All,
>
> Note: This is *not* an iSeries specific question, so please don't let your 
> eyes glaze over at the start :-)
>   
Yes, yes ...

Anyway: Looking at this from a strictly standards-compliance point of 
view, this is certainly a bug in your compiler. The C standard (both C90 
and C99) allow enum types to be the smallest size that can represent all 
the enumeration constants, provided that

    * this type is the same as one of the standard integral types (char,
      short, int, long, long long),
    * that the type can represent the enumeration constants and all
      their bitwise-ored combinations

In other words, the intent of the standard is that bitwise logical 
operations with enumeration constants work as expected, regardless of 
the exact size of the enumeration type.

[...]

> "...enumerated data types in Standard C...act as little more than slightly 
> readable ways to name integer constants.  As a matter of style, we suggest 
> that programmers treat enumerated types as different from integers and not 
> mix them in integer expressions without using casts.  In fact, some UNIX C 
> compilers implement a weakly typed form of enumerations in which some 
> conversions between enumerated types and integers are not permitted 
> without casts."
>   
All such compilers are broken. For one thing, regardless of the enum 
type itself, in C the enumeration _constants_ defined in the enum have 
type int! Also, when mixing enum-typed variables with other integral 
types in an expression, standard integral type promotion rules apply 
(that's one reason for the requirement that the actual type of the enum 
be equivalent to one of the standard integral types).

(Note that C++ is a different beast, with stronger typing rules for 
enums. But we're not concerned with C++ here.)


I think your solution to force the compiler to always use int-sized 
enums is correct in this case. The Subversion code conforms to the 
standard in this particular instance.

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org