On Mon, Jan 22, 2024 at 8:55 AM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> Provide more descriptive error messages by including the identifier
> or other kind of value if available.
>
> Also drop duplicate newlines at the end of messages.
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
Acked-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> checkpolicy/module_compiler.c | 6 +-
> checkpolicy/policy_define.c | 123 ++++++++++++++++++++--------------
> 2 files changed, 76 insertions(+), 53 deletions(-)
>
> diff --git a/checkpolicy/module_compiler.c b/checkpolicy/module_compiler.c
> index 74a9f93c..119b7e36 100644
> --- a/checkpolicy/module_compiler.c
> +++ b/checkpolicy/module_compiler.c
> @@ -75,7 +75,7 @@ static void print_error_msg(int ret, uint32_t symbol_type)
> yyerror2("Could not declare %s here", flavor_str[symbol_type]);
> break;
> default:
> - yyerror("Unknown error");
> + yyerror2("Unknown error %d", ret);
> }
> }
>
> @@ -86,7 +86,7 @@ int define_policy(int pass, int module_header_given)
> if (module_header_given) {
> if (policydbp->policy_type != POLICY_MOD) {
> yyerror
> - ("Module specification found while not building a policy module.\n");
> + ("Module specification found while not building a policy module.");
> return -1;
> }
>
> @@ -111,7 +111,7 @@ int define_policy(int pass, int module_header_given)
> } else {
> if (policydbp->policy_type == POLICY_MOD) {
> yyerror
> - ("Building a policy module, but no module specification found.\n");
> + ("Building a policy module, but no module specification found.");
> return -1;
> }
> }
> diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> index 360cff68..cd49cae3 100644
> --- a/checkpolicy/policy_define.c
> +++ b/checkpolicy/policy_define.c
> @@ -96,6 +96,17 @@ void yyerror2(const char *fmt, ...)
> va_end(ap);
> }
>
> +__attribute__ ((format(printf, 1, 2)))
> +static void yywarn2(const char *fmt, ...)
> +{
> + char warnmsg[256];
> + va_list ap;
> + va_start(ap, fmt);
> + vsnprintf(warnmsg, sizeof(warnmsg), fmt, ap);
> + yywarn(warnmsg);
> + va_end(ap);
> +}
> +
> int insert_separator(int push)
> {
> int error;
> @@ -233,7 +244,7 @@ int define_permissive(void)
> }
>
> if (t->flavor == TYPE_ATTRIB) {
> - yyerror2("attributes may not be permissive: %s\n", type);
> + yyerror2("attributes may not be permissive: %s", type);
> rc = -1;
> goto out;
> }
> @@ -520,7 +531,7 @@ int define_common_perms(void)
> }
> comdatum = hashtab_search(policydbp->p_commons.table, id);
> if (comdatum) {
> - yyerror2("duplicate declaration for common %s\n", id);
> + yyerror2("duplicate declaration for common %s", id);
> free(id);
> return -1;
> }
> @@ -557,8 +568,8 @@ int define_common_perms(void)
> perdatum->s.value = comdatum->permissions.nprim + 1;
>
> if (perdatum->s.value > (sizeof(sepol_access_vector_t) * 8)) {
> - yyerror
> - ("too many permissions to fit in an access vector");
> + yyerror2
> + ("too many permissions (%d) to fit in an access vector", perdatum->s.value);
> goto bad_perm;
> }
> ret = hashtab_insert(comdatum->permissions.table,
> @@ -619,12 +630,15 @@ int define_av_perms(int inherits)
> yyerror2("class %s is not defined", id);
> goto bad;
> }
> - free(id);
>
> if (cladatum->comdatum || cladatum->permissions.nprim) {
> - yyerror("duplicate access vector definition");
> - return -1;
> + yyerror2("duplicate access vector definition for class %s", id);
> + goto bad;
> }
> +
> + free(id);
> + id = NULL;
> +
> if (symtab_init(&cladatum->permissions, PERM_SYMTAB_SIZE)) {
> yyerror("out of memory");
> return -1;
> @@ -664,8 +678,8 @@ int define_av_perms(int inherits)
> perdatum->s.value = ++cladatum->permissions.nprim;
>
> if (perdatum->s.value > (sizeof(sepol_access_vector_t) * 8)) {
> - yyerror
> - ("too many permissions to fit in an access vector");
> + yyerror2
> + ("too many permissions (%d) to fit in an access vector", perdatum->s.value);
> goto bad;
> }
> if (inherits) {
> @@ -737,7 +751,7 @@ int define_sens(void)
> return -1;
> }
> if (id_has_dot(id)) {
> - yyerror("sensitivity identifiers may not contain periods");
> + yyerror2("sensitivity identifier %s may not contain periods", id);
> goto bad;
> }
> level = (mls_level_t *) malloc(sizeof(mls_level_t));
> @@ -766,11 +780,11 @@ int define_sens(void)
> goto bad;
> }
> case -2:{
> - yyerror("duplicate declaration of sensitivity level");
> + yyerror2("duplicate declaration of sensitivity level %s", id);
> goto bad;
> }
> case -1:{
> - yyerror("could not declare sensitivity level here");
> + yyerror2("could not declare sensitivity level %s here", id);
> goto bad;
> }
> case 0:
> @@ -784,7 +798,7 @@ int define_sens(void)
>
> while ((id = queue_remove(id_queue))) {
> if (id_has_dot(id)) {
> - yyerror("sensitivity aliases may not contain periods");
> + yyerror2("sensitivity alias %s may not contain periods", id);
> free(id);
> return -1;
> }
> @@ -806,13 +820,13 @@ int define_sens(void)
> goto bad_alias;
> }
> case -2:{
> - yyerror
> - ("duplicate declaration of sensitivity alias");
> + yyerror2
> + ("duplicate declaration of sensitivity alias %s", id);
> goto bad_alias;
> }
> case -1:{
> - yyerror
> - ("could not declare sensitivity alias here");
> + yyerror2
> + ("could not declare sensitivity alias %s here", id);
> goto bad_alias;
> }
> case 0:
> @@ -920,7 +934,7 @@ int define_category(void)
> return -1;
> }
> if (id_has_dot(id)) {
> - yyerror("category identifiers may not contain periods");
> + yyerror2("category identifier %s may not contain periods", id);
> goto bad;
> }
> datum = (cat_datum_t *) malloc(sizeof(cat_datum_t));
> @@ -938,11 +952,11 @@ int define_category(void)
> goto bad;
> }
> case -2:{
> - yyerror("duplicate declaration of category");
> + yyerror2("duplicate declaration of category %s", id);
> goto bad;
> }
> case -1:{
> - yyerror("could not declare category here");
> + yyerror2("could not declare category %s here", id);
> goto bad;
> }
> case 0:
> @@ -957,7 +971,7 @@ int define_category(void)
>
> while ((id = queue_remove(id_queue))) {
> if (id_has_dot(id)) {
> - yyerror("category aliases may not contain periods");
> + yyerror2("category alias %s may not contain periods", id);
> free(id);
> return -1;
> }
> @@ -980,13 +994,13 @@ int define_category(void)
> goto bad_alias;
> }
> case -2:{
> - yyerror
> - ("duplicate declaration of category aliases");
> + yyerror2
> + ("duplicate declaration of category alias %s", id);
> goto bad_alias;
> }
> case -1:{
> - yyerror
> - ("could not declare category aliases here");
> + yyerror2
> + ("could not declare category alias %s here", id);
> goto bad_alias;
> }
> case 0:
> @@ -1114,7 +1128,7 @@ int define_level(void)
> range_end = cdatum->s.value - 1;
>
> if (range_end < range_start) {
> - yyerror2("category range is invalid");
> + yyerror2("category range %d-%d is invalid", range_start, range_end);
> free(id);
> return -1;
> }
> @@ -1170,6 +1184,7 @@ int expand_attrib(void)
> ebitmap_t attrs;
> type_datum_t *attr;
> ebitmap_node_t *node;
> + const char *name;
> uint32_t i;
> int rc = -1;
> int flags = 0;
> @@ -1222,13 +1237,13 @@ int expand_attrib(void)
> }
>
> ebitmap_for_each_positive_bit(&attrs, node, i) {
> - attr = hashtab_search(policydbp->p_types.table,
> - policydbp->sym_val_to_name[SYM_TYPES][i]);
> + name = policydbp->sym_val_to_name[SYM_TYPES][i];
> + attr = hashtab_search(policydbp->p_types.table, name);
> attr->flags |= flags;
> if ((attr->flags & TYPE_FLAGS_EXPAND_ATTR_TRUE) &&
> (attr->flags & TYPE_FLAGS_EXPAND_ATTR_FALSE)) {
> - yywarn("Expandattribute option was set to both true and false. "
> - "Resolving to false.");
> + yywarn2("Expandattribute option of attribute %s was set to both true and false; "
> + "Resolving to false.", name);
> attr->flags &= ~TYPE_FLAGS_EXPAND_ATTR_TRUE;
> }
> }
> @@ -1247,9 +1262,9 @@ static int add_aliases_to_type(type_datum_t * type)
> int ret;
> while ((id = queue_remove(id_queue))) {
> if (id_has_dot(id)) {
> + yyerror2
> + ("type alias identifier %s may not contain periods", id);
> free(id);
> - yyerror
> - ("type alias identifiers may not contain periods");
> return -1;
> }
> aliasdatum = (type_datum_t *) malloc(sizeof(type_datum_t));
> @@ -1274,7 +1289,7 @@ static int add_aliases_to_type(type_datum_t * type)
> goto cleanup;
> }
> case -1:{
> - yyerror("could not declare alias here");
> + yyerror2("could not declare alias %s here", id);
> goto cleanup;
> }
> case 0: break;
> @@ -1790,8 +1805,8 @@ int define_bool_tunable(int is_tunable)
> return -1;
> }
> if (id_has_dot(id)) {
> + yyerror2("boolean identifier %s may not contain periods", id);
> free(id);
> - yyerror("boolean identifiers may not contain periods");
> return -1;
> }
> datum = (cond_bool_datum_t *) malloc(sizeof(cond_bool_datum_t));
> @@ -1814,7 +1829,7 @@ int define_bool_tunable(int is_tunable)
> goto cleanup;
> }
> case -1:{
> - yyerror("could not declare boolean here");
> + yyerror2("could not declare boolean %s here", id);
> goto cleanup;
> }
> case 0:
> @@ -1957,7 +1972,8 @@ static int avrule_read_ioctls(struct av_ioctl_range_list **rangehead)
> id = queue_remove(id_queue);
> r->range.high = (uint16_t) strtoul(id,NULL,0);
> if (r->range.high < r->range.low) {
> - yyerror("Ioctl ranges must be in ascending order.");
> + yyerror2("Ioctl range %d-%d must be in ascending order.",
> + r->range.low, r->range.high);
> return -1;
> }
> free(id);
> @@ -2532,7 +2548,7 @@ int define_te_avtab_extended_perms(int which)
> if (strcmp(id,"ioctl") == 0) {
> rc = define_te_avtab_ioctl(avrule_template);
> } else {
> - yyerror("only ioctl extended permissions are supported");
> + yyerror2("only ioctl extended permissions are supported, found %s", id);
> rc = -1;
> }
>
> @@ -3189,7 +3205,7 @@ int define_role_allow(void)
> avrule_t *define_cond_filename_trans(void)
> {
> yyerror("type transitions with a filename not allowed inside "
> - "conditionals\n");
> + "conditionals");
> return COND_ERR;
> }
>
> @@ -3861,7 +3877,7 @@ int define_conditional(cond_expr_t * expr, avrule_t * t, avrule_t * f)
> f = 0;
> expr = define_cond_expr(COND_NOT, expr, 0);
> if (!expr) {
> - yyerror("unable to invert");
> + yyerror("unable to invert conditional expression");
> return -1;
> }
> }
> @@ -4126,7 +4142,7 @@ static int parse_categories(char *id, level_datum_t * levdatum, ebitmap_t * cats
> range_end = cdatum->s.value - 1;
>
> if (range_end < range_start) {
> - yyerror2("category range is invalid");
> + yyerror2("category range %d-%d is invalid", range_start, range_end);
> return -1;
> }
> } else {
> @@ -5102,7 +5118,7 @@ int define_ibendport_context(unsigned int port)
> }
>
> if (port > 0xff || port == 0) {
> - yyerror("Invalid ibendport port number, should be 0 < port < 256");
> + yyerror2("Invalid ibendport port number %d, should be 0 < port < 256", port);
> return -1;
> }
>
> @@ -5121,7 +5137,7 @@ int define_ibendport_context(unsigned int port)
> }
>
> if (strlen(newc->u.ibendport.dev_name) > IB_DEVICE_NAME_MAX - 1) {
> - yyerror("infiniband device name exceeds max length of 63.");
> + yyerror2("infiniband device name %s exceeds max length of 63.", newc->u.ibendport.dev_name);
> rc = -1;
> goto out;
> }
> @@ -5248,13 +5264,14 @@ int define_ipv4_node_context(void)
> }
>
> rc = inet_pton(AF_INET, id, &addr);
> - free(id);
> if (rc < 1) {
> - yyerror("failed to parse ipv4 address");
> + yyerror2("failed to parse ipv4 address %s", id);
> + free(id);
> if (rc == 0)
> rc = -1;
> goto out;
> }
> + free(id);
>
> id = queue_remove(id_queue);
> if (!id) {
> @@ -5264,14 +5281,16 @@ int define_ipv4_node_context(void)
> }
>
> rc = inet_pton(AF_INET, id, &mask);
> - free(id);
> if (rc < 1) {
> - yyerror("failed to parse ipv4 mask");
> + yyerror2("failed to parse ipv4 mask %s", id);
> + free(id);
> if (rc == 0)
> rc = -1;
> goto out;
> }
>
> + free(id);
> +
> if (mask.s_addr != 0 && ((~mask.s_addr + 1) & ~mask.s_addr) != 0) {
> yywarn("ipv4 mask is not contiguous");
> }
> @@ -5376,14 +5395,16 @@ int define_ipv6_node_context(void)
> }
>
> rc = inet_pton(AF_INET6, id, &addr);
> - free(id);
> if (rc < 1) {
> - yyerror("failed to parse ipv6 address");
> + yyerror2("failed to parse ipv6 address %s", id);
> + free(id);
> if (rc == 0)
> rc = -1;
> goto out;
> }
>
> + free(id);
> +
> id = queue_remove(id_queue);
> if (!id) {
> yyerror("failed to read ipv6 address");
> @@ -5392,14 +5413,16 @@ int define_ipv6_node_context(void)
> }
>
> rc = inet_pton(AF_INET6, id, &mask);
> - free(id);
> if (rc < 1) {
> - yyerror("failed to parse ipv6 mask");
> + yyerror2("failed to parse ipv6 mask %s", id);
> + free(id);
> if (rc == 0)
> rc = -1;
> goto out;
> }
>
> + free(id);
> +
> if (!ipv6_is_mask_contiguous(&mask)) {
> yywarn("ipv6 mask is not contiguous");
> }
> --
> 2.43.0
>
>
