Svante,
I have some additional comments. Please see below.
On 15/05/14 08:43, Svante Signell wrote:
>
> static void
> -mkfifofn(char *restrict buf, size_t bsz, const char *key, unsigned int tid)
> +mkfifofn(char **buf, const char *key, unsigned int tid)
> {
> - snprintf(buf, bsz, "%s output %x", key, tid);
> + size_t len = strlen(key) + 9 + 8 + 1;
> + *buf = malloc(len);
> + if (*buf == NULL)
> + fprintf(stderr, "malloc failed\n");
> + snprintf(*buf, len, "%s output %x", key, tid);
If the malloc fails before, you'll dereference a NULL pointer here.
Instead of passing a char**, you should probably allocate and return the
formatted fifo name instead and adjust the use accordingly (see below):
static char*
mkfifofn(const char *key, unsigned int tid)
{
char *buf = NULL;
size_t len = strlen(key) + 9 + 8 + 1;
buf = malloc(len);
if (buf)
snprintf(buf, len, "%s output %x", key, tid);
return buf;
}
>
> @@ -670,23 +674,32 @@ differ(struct clit_chld_s ctx[static 1],
> #if !defined L_tmpnam
> # define L_tmpnam (PATH_MAX)
> #endif /* !L_tmpnam */
> - static char expfn[PATH_MAX];
> - static char actfn[PATH_MAX];
Why not simply replace PATH_MAX with L_tmpnam and be done with
everything? Is this defined on GNU/Hurd? Is it used elsewhere in the file?
> + static char *expfn = NULL;
> + static char *actfn = NULL;
Why keep them static?
> + if (clit_bit_fn_p(exp)) {
> + expfn = malloc(strlen(exp.d) + 1);
> + if (expfn == NULL) {
> + error( "malloc failed\n");
> + goto out;
> + }
> + if (strcpy(expfn, exp.d) == NULL) {
> + error("cannot prepare in file `%s'", exp.d);
> + goto out;
> + }
It's probably not necessary to explicitly distinguish these two errors.
I would just use:
if (expfn == NULL || strcpy(expfn, exp.d) == NULL)
error("cannot prepare in file `%s'", exp.d);
> + if (!clit_bit_fn_p(exp)) {
> + if (mkfifofn(&expfn, "expected", ctx->test_id),
> + mkfifo(expfn, 0666) < 0) {
In your patch, mkfifofn can fail. By adopting my suggestion to return
the allocated and formatted name instead, you can use an || instead of
the comma operator here:
if (!(expfn = mkfifofn("expected", ctx->test_id)) ||
mkfifo(expfn, 0666) < 0) {
Alternatively, and maybe a easier to read, you should use
expfn = mkfifofn("expected", ctx->test_id);
if (expfn == NULL || mkfifo(expfn, 0666) < 0) {
> + if (mkfifofn(&actfn, "actual", ctx->test_id),
> + mkfifo(actfn, 0666) < 0) {
Same here:
actfn = mkfifofn("actual", ctx->test_id);
if (actfn == NULL || mkfifo(actfn, 0666) < 0) {
> out:
> - if (*expfn && !clit_bit_fn_p(exp)) {
> - unlink(expfn);
> + if (*expfn) {
Here, you need to check expfn itself, because it may not have been
allocated. Based on above's mkfifofn() implementation, it will then
always start with a non-NULL element.
> + if (!clit_bit_fn_p(exp))
> + unlink(expfn);
> + free(expfn);
> }
Indentation broken? Or did you mean to free(expfn) outside of the condition?
> if (*actfn) {
> unlink(actfn);
> + free(actfn);
> }
Same here, you need to check actfn, as actfn might not have been allocated.
My 2ยข,
Philipp
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
Archive: https://lists.debian.org/[email protected]
