Page MenuHomeFreeBSD

Do not depend on stdin.
ClosedPublic

Authored by oshogbo on Nov 18 2018, 3:07 PM.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem accepted this revision.EditedNov 18 2018, 11:30 PM

I like the change. All of my comments below describe existing issues in the code, nothing you introduced in this change. Maybe they can be addressed separately.

contrib/elftoolchain/strings/strings.c
249–251 ↗(On Diff #50561)

I think this check can be removed.

254 ↗(On Diff #50561)

This memset is unnecessary

298 ↗(On Diff #50561)

This return type is bogus on 32-bit arch (sizeof(long) == 4).

EOF (-1) is indistinguishable from a valid 32-bit value.

306–310 ↗(On Diff #50561)

Isn't this bogus? It will spuriously fail to read the last byte at the end of the file. It also mishandles stream errors prior to EOF.

I suggest:

c = getc(pfile);
if (c == EOF)
    return (EOF);

instead.

325–330 ↗(On Diff #50561)

This math is undefined behavior on 32-bit long systems, I think (left shift into sign bit).

Probably the "value" result should be a 32-bit unsigned integer.

And the return type can either be a wider signed type (int64_t) or the error status can be separated from the return value (i.e., uint32_t *value output parameter).

365 ↗(On Diff #50561)

I don't think there's any reason to keep this conditional on feof(pfile). EOF could also indicate an IO error rather than actual end of file. We don't want to continue if getcharacter() returned EOF regardless of anything else.

412–417 ↗(On Diff #50561)

EOF check should be done before (uint8_t)c > 127 test, which for c == EOF will always succeed.

This revision is now accepted and ready to land.Nov 18 2018, 11:30 PM

I like this change.

Adding @kaiw and @jkoshy_users.sourceforge.net to CC as a heads-up as we will want this to go upstream as well. (I can take care of the upstream commit.)

contrib/elftoolchain/strings/strings.c
249–251 ↗(On Diff #50561)

Thanks. You are right I will address that.

254 ↗(On Diff #50561)

Thanks. You are right I will address that.

298 ↗(On Diff #50561)

Thanks. You are right I will fix that as separate commit.

306–310 ↗(On Diff #50561)

Thanks. You are right I will fix that as separate commit.

325–330 ↗(On Diff #50561)

Thanks. You are right I will fix that as separate commit.

365 ↗(On Diff #50561)

Thanks. You are right I will fix that as separate commit.

412–417 ↗(On Diff #50561)

Thanks. You are right I will fix that as separate commit.

This revision was automatically updated to reflect the committed changes.