New gets_s(3) stdio function

[cy]

Summary

This revision implements the gets_s(3) function as documented at http://en.cppreference.com/w/c/io/gets. It facilitates the optional/subsequent removal of gets(3).

Test Plan

I've been using this function in libc on my systems for the past few weeks. However callout to __throw_constraint_handler_s remains to be tested.

[ed]

I'm personally not a fan of the *_s() functions. That said, we've already added some others to the base system, so there's probably no turning back.

lib/libc/stdio/gets.c

49

This comment should likely be removed, right? _gets_s() is not part of ISO C.

51

static and char * tend to be placed on a single line.

58

bool gets_s_call = n != NULL;

There is no need to resort to preprocessor logic here.

59

Unnecessary spaces inside the parentheses.

66

Multi-line comments should be written as follows, per style(9):

/*
 * ...
 * ...
 */

91

There is not a single block of code that is unaffected by GETS_S_CALL. My question then becomes, shouldn't we just make these two separate functions?

121

Missing space between return and (

[asomers]

lib/libc/stdio/gets.c

126

How about a consider using gets_s(3) instead. ?

[imp]

You need to fix the namespace pollution, and really, make this two separate functions. The current structure tries to be too clever, and history has shown that this will result in subtle bugs and possibly a security advisory.

include/stdio.h

39

Doesn't this introduce unacceptable namespace pollution? Aren't we using the _foo.h files to specifically avoid it. The standard is quite strict about what's allowed to be defined, and all of stddef.h isn't in the list for stdio.h. This must be reverted before the commit.

lib/libc/stdio/gets.c

63–66

This comment is incoherent and misleading.

74

Why not just have two functions? It makes no sense at all to make them be the same if they are this different.

This construct is overly complicated and once you simplify it to something sane and reasonable, you're left with so little code in common that it makes no sense to share it. The attempt to share the code makes it much worse in this case.

91

Definitely.

[cy]

To address the problem with __warn_references and the overly complicated logic, this will need to become a separate gets_s.c file.

Investigate rsize_t move to reduce namespace pollution.

And gcc6 build failure in poudriere.

Thanks for all the input so far.

include/stdio.h

39

We may need to move the rsize_t definition then. I'll take a look at this.

lib/libc/stdio/gets.c

49

Yes. _gets_s() is the internal function.

51

Sure.

58

Agreed.

59

Agreed.

74

I was toying with that when I last looked at it last night.

91

I'll rewrite it into a separate function. Additionally, I have a poudriere build that has a bit of gas with my latest version: gcc6.

126

This is why I was toying with splitting this into another file. There is no way to warn about gets(3) and not have a warning with gets_s(3). This has to go into a separate file.

[ed]

include/stdio.h

39

If the C standard allows <stdio.h> to define rsize_t, just copy-paste this from <stddef.h> into <stdio.h>:

#ifndef _RSIZE_T_DEFINED                                                         
#define _RSIZE_T_DEFINED                                                         
typedef size_t rsize_t;                                                          
#endif

If not, then simply use size_t in the prototype in the header file and rsize_t in the source file. That way you do get a compiler error if the two go out of sync.

[bjk]

lib/libc/stdio/fgets.3

50

Please remember to update .Dd when committing.

[cy]

The gets_s() has been separated out into a different file.

Would it be better to svn copy & update or a new file from scratch?

[ed]

In D12785#267382, @cy wrote:

The gets_s() has been separated out into a different file.

Would it be better to svn copy & update or a new file from scratch?

It depends. If svn diff still returns something sensible after copying, then doing a svn copy is the right approach. If the contents of the file are almost entirely different and svn diff would return more differences than what's retained, a new file is the right approach.

[cy]

I'll switch to svn from git and see what the diff gives me.

Other than that, what do people think?

lib/libc/stdio/gets.c

74

It's been separated out.

[ed]

Looks a lot better. Thanks!

Could I persuade you to also add some basic unit testing coverage for this function?

lib/libc/stdio/gets_s.c

17 ↗

(On Diff #34576)

Do you want this code to be 3-clause BSD licensed on your behalf as well? If so, leave this as is. Otherwise, remove your name from the copyright above and add a second (2-clause BSD?) license to this source file.

69 ↗

(On Diff #34576)

I'd suggest putting { }'s around this block to prevent potential 'trailing else' issues from being introduced when refactoring.

80 ↗

(On Diff #34576)

There is no need to have these empty lined in the comment block, right?

85 ↗

(On Diff #34576)

Unnecessary space.

87 ↗

(On Diff #34576)

You can remove one pair of parentheses here.

[cy]

You've persuaded me to write a testsuite. What I'm currently using to test is not of any quality to commit. However, I'll be out of town the next week and a half but I'll possibly have an hour each evening to work at it. Thanks for the input.

lib/libc/stdio/fgets.3

50

I'll update it again just prior to commit.

lib/libc/stdio/gets_s.c

17 ↗

(On Diff #34576)

I think I'll do that. I won't mark this done until it is.

69 ↗

(On Diff #34576)

Fixed.

[jhb]

lib/libc/stdio/gets_s.c

56 ↗

(On Diff #34576)

Style nit: space after return (several times)

88 ↗

(On Diff #34576)

I actually think this is simpler if you just avoid the goto and duplicate the one line to unlock:

FUNLOCKFILE_CANCELSAFE();
__throw_constraint_handler_s("gets_s : end of buffer", E2BIG);
return (NULL);

You can then remove the 'throw_error' variable and the 'end:' label.

[cy]

Unfortunately FUNLOCKFILE_CANCELSAFE and FLOCKFILE_CANCELSAFE implement an ugly lock. The macros must be coded as matching pairs and to remove the goto and replace it wtih another FUNLOCFILE_CANCELSAFE would result in nasty compile errors. They are used as pairs.

I looked at replacing the macros a couple of weeks ago but I think we need to limit the scope of this revision and consider replacing these macros, used throughout stdio, in another revision. Do you agree?

#define FLOCKFILE_CANCELSAFE(fp) \

{                                                               \
        struct _pthread_cleanup_info __cleanup_info__;          \
        if (__isthreaded) {                                     \
                _FLOCKFILE(fp);                                 \
                ___pthread_cleanup_push_imp(                    \
                    __stdio_cancel_cleanup, (fp),               \
                    &__cleanup_info__);                         \
        } else {                                                \
                ___pthread_cleanup_push_imp(                    \
                    __stdio_cancel_cleanup, NULL,               \
                    &__cleanup_info__);                         \
        }                                                       \
        {

#define FUNLOCKFILE_CANCELSAFE() \

                (void)0;                                        \
        }                                                       \
        ___pthread_cleanup_pop_imp(1);                          \
}

[jhb]

Ok, just leave the goto as-is then. I don't think it is worth it to try to replace those macros. pthread_cleanup_push/pop are themselves implemented as paired macros (POSIX permits this), so any wrappers of those macros will be as well.

[cy]

I had a chance to think about the macros on the way to $JOB just after commenting here. We can address this by using a static inline function. It's not ideal but IMO better than goto's.

We can address the macro mess properly in the rest of stdio in another revision.

This new revision includes a test. I cobbled it up last night. It hasn't been tested yet. Better to give you a chance to review it now while I test over the weekend.

[ed]

lib/libc/tests/stdio/gets_s_test.c

31 ↗

(On Diff #35692)

This include needs to be in a separate block above <assert.h>.

33 ↗

(On Diff #35692)

Doesn't this need to be <sys/wait.h>? Do we even provide a header with that name?

38 ↗

(On Diff #35692)

Could these have more descriptive names? Also no need to put restrict here. This keyword only makes sense on function arguments.

51 ↗

(On Diff #35692)

Wouldn't it be better to use ATF_CHECK*() here?

65 ↗

(On Diff #35692)

That == is not intended, right? Also: missing whitespace between >= and 0.

[cy]

lib/libc/tests/stdio/gets_s_test.c

31 ↗

(On Diff #35692)

Sorry about this. I was in a bit of a hurry to put it out there prior to tinderbox and testing.

38 ↗

(On Diff #35692)

That had crossed my mind.

65 ↗

(On Diff #35692)

Oops.

[cy]

It's been a while but I think this should address all the comments and concerns previously noted.

[wblock]

lib/libc/stdio/fgets.3

97

Use the serial comma:

.Fn gets_s ,

112

As above:

.Fn gets_s ,

[cy]

I will upload fixes.

[cy]

This addresses two punctuation issues wblock raised.

[cy]

In D12785#265442, @imp wrote:

You need to fix the namespace pollution, and really, make this two separate functions. The current structure tries to be too clever, and history has shown that this will result in subtle bugs and possibly a security advisory.

Addressed in a prior revision.

[cy]

Are there any more comments or requested revisions?

[cy]

I suppose that since there are no more comments, I'll commit this sometime after April 4 (after I get back).

[ed]

Looks good to me. Thanks!

lib/libc/tests/stdio/gets_s_test.c

33 ↗

(On Diff #40025)

The <sys/...> headers need to be above the others.

[emaste]

lib/libc/stdio/fgets.3

50

a post-commit comment: should gets_s also be in the title?

.Sh NAME
.Nm fgets ,
.Nm gets ,
.Nm gets_s
.Nd get a line from a stream

noticed while merging this to my working branch which has plain gets removed.

[cy]

Indeed it should and the update date should be updated as well. I'll get on it.