Re: dateutils: FTBFS on hurd-i386 (for review)
On Thu, 2014-05-15 at 13:38 +0200, Philipp A. Hartmann wrote:
> 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):
Yes, you are right. However, setting len to zero would have solved that.
The main problem is that no return value is given for the function so
your solution is better.
> 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?
No and no.
> > + static char *expfn = NULL;
> > + static char *actfn = NULL;
>
> Why keep them static?
Why not?
> > + 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);
Fixed
> > + 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) {
Fixed
> > + 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) {
Ditto
> > 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.
Fixed
> > + if (!clit_bit_fn_p(exp))
> > + unlink(expfn);
> > + free(expfn);
> > }
>
> Indentation broken? Or did you mean to free(expfn) outside of the condition?
Yes outside the condition. expfn is used for both cases
clit_bit_fn_p(exp) and !clit_bit_fn_p(exp). expfn is a fifo only for
case !clit_bit_fn_p(exp). Found that out via valgrind.
> > if (*actfn) {
> > unlink(actfn);
> > + free(actfn);
> > }
>
> Same here, you need to check actfn, as actfn might not have been allocated.
Ditto
Thanks!
--- a/test/clittool.c
+++ b/test/clittool.c
@@ -619,11 +619,18 @@ xclosefrom(int fd)
return;
}
-static void
-mkfifofn(char *restrict buf, size_t bsz, const char *key, unsigned int tid)
+static char*
+mkfifofn(const char *key, unsigned int tid)
{
- snprintf(buf, bsz, "%s output %x", key, tid);
- return;
+ char *buf = NULL;
+ size_t len = strlen(key) + 9 + 8 + 1;
+ buf = malloc(len);
+ if (buf == NULL) {
+ len = 0;
+ fprintf(stderr, "malloc failed\n");
+ }
+ snprintf(buf, len, "%s output %x", key, tid);
+ return buf;
}
static pid_t
@@ -670,27 +677,31 @@ 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];
+ static char *expfn = NULL;
+ static char *actfn = NULL;
pid_t difftool = -1;
assert(!clit_bit_fd_p(exp));
- if (clit_bit_fn_p(exp) &&
- (strlen(exp.d) >= sizeof(expfn) || strcpy(expfn, exp.d) == NULL)) {
- error("cannot prepare in file `%s'", exp.d);
- goto out;
- } else if (!clit_bit_fn_p(exp) &&
- (mkfifofn(expfn, sizeof(expfn), "expected", ctx->test_id),
- mkfifo(expfn, 0666) < 0)) {
- error("cannot create fifo `%s'", expfn);
- goto out;
- } else if (mkfifofn(actfn, sizeof(actfn), "actual", ctx->test_id),
- mkfifo(actfn, 0666) < 0) {
+ if (clit_bit_fn_p(exp)) {
+ expfn = malloc(strlen(exp.d) + 1);
+ if (expfn == NULL || strcpy(expfn, exp.d) == NULL) {
+ error("cannot prepare in file `%s'", exp.d);
+ goto out;
+ }
+ }
+ if (!clit_bit_fn_p(exp)) {
+ expfn = mkfifofn("expected", ctx->test_id);
+ if ( expfn == NULL || mkfifo(expfn, 0666) < 0) {
+ error("cannot create fifo `%s'", expfn);
+ goto out;
+ }
+ }
+ actfn = mkfifofn("actual", ctx->test_id);
+ if ( actfn == NULL || mkfifo(actfn, 0666) < 0) {
error("cannot create fifo `%s'", actfn);
goto out;
}
-
block_sigs();
switch ((difftool = vfork())) {
@@ -701,7 +712,7 @@ differ(struct clit_chld_s ctx[static 1],
case 0:;
/* i am the child */
- static char *const diff_opt[] = {
+ char *const diff_opt[] = {
"diff",
"-u",
expfn, actfn, NULL,
@@ -761,11 +772,14 @@ differ(struct clit_chld_s ctx[static 1],
unblock_sigs();
out:
- if (*expfn && !clit_bit_fn_p(exp)) {
- unlink(expfn);
+ if (expfn) {
+ if (!clit_bit_fn_p(exp))
+ unlink(expfn);
+ free(expfn);
}
- if (*actfn) {
+ if (actfn) {
unlink(actfn);
+ free(actfn);
}
return difftool;
}
Reply to: