Page MenuHomePhabricator

Avoid use of gets() in DOE-ProxyApps-C/Pathfinder
Needs ReviewPublic

Authored by arichardson on Oct 16 2020, 8:43 AM.

Details

Reviewers
jrtc27
dim
Summary

gets() has been removed from C11 and on FreeBSD 13 libc no longer provides
it by default. Use fgets() instead.

Event Timeline

arichardson created this revision.Oct 16 2020, 8:43 AM
arichardson requested review of this revision.Oct 16 2020, 8:43 AM
jrtc27 requested changes to this revision.Oct 17 2020, 7:17 AM
jrtc27 added a subscriber: jrtc27.
jrtc27 added inline comments.
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

gets always removes the newline, but fgets will include it (except in the case where the input reaches EOF before one is found) so the final character needs to be removed if it's \n.

This revision now requires changes to proceed.Oct 17 2020, 7:17 AM

remove newline

arichardson marked an inline comment as done.Oct 18 2020, 10:51 AM
krytarowski added inline comments.Oct 20 2020, 2:57 PM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

Can you introduce mygets that implements gets_s internally?

arichardson added inline comments.Oct 21 2020, 6:39 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

I could do, but that seems like overkill for the one use of gets().

krytarowski added inline comments.Oct 21 2020, 6:44 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303
static char *mygets(char *str, size_t n)
{
    char *ret;
    size_t last;

    if ((ret = fgets(str, n, stdin)) != NULL) {
        last = strlen(str) - 1;

        if (str[last] == '\n' || str[last] == '\r' )
            str[last] = '\0';
    }

    return ret;
}

And then use mygets(stringBuffer, sizeof(stringBuffer));

It looks nicer to me.

arichardson added inline comments.Oct 21 2020, 7:07 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

That will not work with zero-length strings (it will access str[-1]),

Adding stringBuffer[strcspn(stringBuffer, "\r\n")] = '\0'; seems much simpler to me.

jrtc27 added inline comments.Oct 21 2020, 7:10 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

Also the \r check is misguided. In text mode, line endings get normalised to just \n. In binary mode they will stay as-is, but on Windows that means you have \r\n (and \r on classic Mac OS 9 and below I guess) so the if would need to be a while and last decremented. But stdin should be in text mode by default.

arichardson added inline comments.Oct 21 2020, 7:12 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

strcspn() returns the length up to the first occurrence of \r or \n, so it should work?

jrtc27 added inline comments.Oct 21 2020, 7:14 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

Yeah, that avoids any such complications. Technically the \r can just be dropped from it I believe but it does no harm (well unless people feed in \r on systems with Unix line endings I guess...).

krytarowski added inline comments.Oct 21 2020, 8:20 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

That will not work with zero-length strings (it will access str[-1]),

How so? fgets returns NULL for 0-length strings.

I find this switch more elegant with gets_s()-like code, but this is a matter of taste.

arichardson added inline comments.Oct 21 2020, 8:26 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

I'd prefer gets_s, but we can't assume that the platform provides it. using fgets+ strcspn is only one additional line and supported everywhere so it's IMO the simpliest solution. If there were more uses of gets(), then a wrapper might make sense.

krytarowski added inline comments.Oct 21 2020, 10:47 AM
MultiSource/Benchmarks/DOE-ProxyApps-C/Pathfinder/main.c
302–303

I'd prefer gets_s, but we can't assume that the platform provides it.

As I proposed, including it inline as mygets or similar, as almost no platforms delivers it.

using fgets+ strcspn is only one additional line and supported everywhere so it's IMO the simpliest solution.

In my personal taste open-coding this looks awful, but this is up to you.

If the LOC would matter in my proposal, mygets could be compressed to 2 lines too.

If there were more uses of gets(), then a wrapper might make sense.

This is fine.