ehlo,
Ensure that cmd_ctx->name will not be NULL.
If cmd_ctx->name was not initialized by sss_parse_name then copy of name will be used.
https://fedorahosted.org/sssd/ticket/1970 Coverity ID: 11647
Patch is attached.
LS
On 07/30/2013 02:42 PM, Lukas Slebodnik wrote:
ehlo,
Ensure that cmd_ctx->name will not be NULL.
If cmd_ctx->name was not initialized by sss_parse_name then copy of name will be used.
https://fedorahosted.org/sssd/ticket/1970 Coverity ID: 11647
Patch is attached.
LS
Nack.
if (cmd_ctx->name == NULL) { cmd_ctx->name = talloc_strdup(cmd_ctx, name); if (!cmd_ctx->name) return ENOMEM; } if (cmd_ctx->is_user && cmd_ctx->domname == NULL) { DEBUG(SSSDBG_TRACE_FUNC, ("Parsing name [%s][%s]\n", name, domain ? domain : "<ALL>"));
You need to talloc_zfree(cmd_ctx->name) before reassigning it.
ret = sss_parse_name_for_domains(cmd_ctx, cctx->rctx->domains, domain, name, &cmd_ctx->domname, &cmd_ctx->name); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, ("Invalid name received [%s]\n", name)); return ENOENT; } } else if (cmd_ctx->domname == NULL) { if (domain != NULL) { cmd_ctx->domname = talloc_strdup(cmd_ctx, domain); if (!cmd_ctx->domname) return ENOMEM; } } if (alias != NULL && strcmp(cmd_ctx->name, alias) != 0) { cmd_ctx->alias = talloc_strdup(cmd_ctx, alias); if (!cmd_ctx->alias) return ENOMEM; }
However, I think it would be read better this way:
if (cmd_ctx->is_user && cmd_ctx->domname == NULL) { ... } else { if (cmd_ctx->name == NULL) { ... }
if (cmd_ctx->domname == NULL && domain != NULL) { ... } }
On (31/07/13 10:57), Pavel Březina wrote:
On 07/30/2013 02:42 PM, Lukas Slebodnik wrote:
ehlo,
Ensure that cmd_ctx->name will not be NULL.
If cmd_ctx->name was not initialized by sss_parse_name then copy of name will be used.
https://fedorahosted.org/sssd/ticket/1970 Coverity ID: 11647
Patch is attached.
LS
Nack.
if (cmd_ctx->name == NULL) { cmd_ctx->name = talloc_strdup(cmd_ctx, name); if (!cmd_ctx->name) return ENOMEM; }
if (cmd_ctx->is_user && cmd_ctx->domname == NULL) { DEBUG(SSSDBG_TRACE_FUNC, ("Parsing name [%s][%s]\n", name, domain ? domain : "<ALL>"));
You need to talloc_zfree(cmd_ctx->name) before reassigning it.
ret = sss_parse_name_for_domains(cmd_ctx, cctx->rctx->domains, domain, name, &cmd_ctx->domname, &cmd_ctx->name); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, ("Invalid name received [%s]\n", name)); return ENOENT; }
} else if (cmd_ctx->domname == NULL) { if (domain != NULL) { cmd_ctx->domname = talloc_strdup(cmd_ctx, domain); if (!cmd_ctx->domname) return ENOMEM; } }
if (alias != NULL && strcmp(cmd_ctx->name, alias) != 0) { cmd_ctx->alias = talloc_strdup(cmd_ctx, alias); if (!cmd_ctx->alias) return ENOMEM; }
However, I think it would be read better this way:
if (cmd_ctx->is_user && cmd_ctx->domname == NULL) { ... } else { if (cmd_ctx->name == NULL) { ... }
if (cmd_ctx->domname == NULL && domain != NULL) { ... } }
Thank you for review, It is a nice catch. I should not write patch anymore in the meeting time.
Updated patch is attached.
LS
On 07/31/2013 02:16 PM, Lukas Slebodnik wrote:
On (31/07/13 10:57), Pavel Březina wrote:
On 07/30/2013 02:42 PM, Lukas Slebodnik wrote:
ehlo,
Ensure that cmd_ctx->name will not be NULL.
If cmd_ctx->name was not initialized by sss_parse_name then copy of name will be used.
https://fedorahosted.org/sssd/ticket/1970 Coverity ID: 11647
Patch is attached.
LS
Nack.
if (cmd_ctx->name == NULL) { cmd_ctx->name = talloc_strdup(cmd_ctx, name); if (!cmd_ctx->name) return ENOMEM; } if (cmd_ctx->is_user && cmd_ctx->domname == NULL) { DEBUG(SSSDBG_TRACE_FUNC, ("Parsing name [%s][%s]\n", name, domain ? domain : "<ALL>"));
You need to talloc_zfree(cmd_ctx->name) before reassigning it.
ret = sss_parse_name_for_domains(cmd_ctx, cctx->rctx->domains, domain, name, &cmd_ctx->domname, &cmd_ctx->name); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, ("Invalid name received [%s]\n", name)); return ENOENT; } } else if (cmd_ctx->domname == NULL) { if (domain != NULL) { cmd_ctx->domname = talloc_strdup(cmd_ctx, domain); if (!cmd_ctx->domname) return ENOMEM; } } if (alias != NULL && strcmp(cmd_ctx->name, alias) != 0) { cmd_ctx->alias = talloc_strdup(cmd_ctx, alias); if (!cmd_ctx->alias) return ENOMEM; }
However, I think it would be read better this way:
if (cmd_ctx->is_user && cmd_ctx->domname == NULL) { ... } else { if (cmd_ctx->name == NULL) { ... }
if (cmd_ctx->domname == NULL && domain != NULL) { ... }
}
Thank you for review, It is a nice catch. I should not write patch anymore in the meeting time.
Updated patch is attached.
LS
Ack.
On Wed, Jul 31, 2013 at 02:31:37PM +0200, Pavel Březina wrote:
On 07/31/2013 02:16 PM, Lukas Slebodnik wrote:
On (31/07/13 10:57), Pavel Březina wrote:
On 07/30/2013 02:42 PM, Lukas Slebodnik wrote:
ehlo,
Ensure that cmd_ctx->name will not be NULL.
If cmd_ctx->name was not initialized by sss_parse_name then copy of name will be used.
https://fedorahosted.org/sssd/ticket/1970 Coverity ID: 11647
Patch is attached.
LS
Nack.
if (cmd_ctx->name == NULL) { cmd_ctx->name = talloc_strdup(cmd_ctx, name); if (!cmd_ctx->name) return ENOMEM; }
if (cmd_ctx->is_user && cmd_ctx->domname == NULL) { DEBUG(SSSDBG_TRACE_FUNC, ("Parsing name [%s][%s]\n", name, domain ? domain : "<ALL>"));
You need to talloc_zfree(cmd_ctx->name) before reassigning it.
ret = sss_parse_name_for_domains(cmd_ctx, cctx->rctx->domains, domain, name, &cmd_ctx->domname, &cmd_ctx->name); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, ("Invalid name received [%s]\n", name)); return ENOENT; }
} else if (cmd_ctx->domname == NULL) { if (domain != NULL) { cmd_ctx->domname = talloc_strdup(cmd_ctx, domain); if (!cmd_ctx->domname) return ENOMEM; } }
if (alias != NULL && strcmp(cmd_ctx->name, alias) != 0) { cmd_ctx->alias = talloc_strdup(cmd_ctx, alias); if (!cmd_ctx->alias) return ENOMEM; }
However, I think it would be read better this way:
if (cmd_ctx->is_user && cmd_ctx->domname == NULL) { ... } else { if (cmd_ctx->name == NULL) { ... }
if (cmd_ctx->domname == NULL && domain != NULL) { ... } }
Thank you for review, It is a nice catch. I should not write patch anymore in the meeting time.
Updated patch is attached.
LS
Ack.
Pushed to master.
sssd-devel@lists.fedorahosted.org