Page MenuHomeFreeBSD

New gets_s(3) stdio function
ClosedPublic

Authored by cy on Oct 25 2017, 6:56 AM.

Details

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.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

cy created this revision.Oct 25 2017, 6:56 AM
cy updated this revision to Diff 34315.Oct 25 2017, 7:06 AM
cy retitled this revision from New stdio gets_s(3) stdio function to New gets_s(3) stdio function.
ed added a comment.Oct 25 2017, 7:21 AM

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 ↗(On Diff #34315)

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

51 ↗(On Diff #34315)

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

58 ↗(On Diff #34315)

bool gets_s_call = n != NULL;

There is no need to resort to preprocessor logic here.

59 ↗(On Diff #34315)

Unnecessary spaces inside the parentheses.

66 ↗(On Diff #34315)

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

/*
 * ...
 * ...
 */
91 ↗(On Diff #34315)

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 ↗(On Diff #34315)

Missing space between return and (

asomers added inline comments.Oct 25 2017, 2:57 PM
lib/libc/stdio/gets.c
126 ↗(On Diff #34315)

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

imp requested changes to this revision.Oct 25 2017, 3:43 PM

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 ↗(On Diff #34315)

This comment is incoherent and misleading.

74 ↗(On Diff #34315)

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 ↗(On Diff #34315)

Definitely.

This revision now requires changes to proceed.Oct 25 2017, 3:43 PM
cy planned changes to this revision.Oct 25 2017, 7:43 PM

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 ↗(On Diff #34315)

Yes. _gets_s() is the internal function.

51 ↗(On Diff #34315)

Sure.

58 ↗(On Diff #34315)

Agreed.

59 ↗(On Diff #34315)

Agreed.

74 ↗(On Diff #34315)

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

91 ↗(On Diff #34315)

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 ↗(On Diff #34315)

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 added inline comments.Oct 25 2017, 8:10 PM
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 added a subscriber: bjk.Oct 25 2017, 10:40 PM
bjk added inline comments.
lib/libc/stdio/fgets.3
50

Please remember to update .Dd when committing.

cy updated this revision to Diff 34576.Nov 1 2017, 2:38 AM
cy marked 11 inline comments as done.

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 added a comment.Nov 1 2017, 8:02 AM
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 added a comment.Nov 1 2017, 1:47 PM

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 ↗(On Diff #34315)

It's been separated out.

ed added a comment.Nov 2 2017, 4:02 PM

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
18

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.

70

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

81

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

86

Unnecessary space.

88

You can remove one pair of parentheses here.

cy planned changes to this revision.Nov 3 2017, 5:48 AM
cy marked 4 inline comments as done.

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
18

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

70

Fixed.

jhb added inline comments.Nov 6 2017, 9:13 PM
lib/libc/stdio/gets_s.c
57

Style nit: space after return (several times)

89

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 marked 2 inline comments as done.Nov 21 2017, 3:03 AM

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 added a comment.Nov 21 2017, 8:03 PM

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 updated this revision to Diff 35692.Nov 24 2017, 3:03 PM
cy added a comment.Nov 24 2017, 3:17 PM

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 added inline comments.Nov 25 2017, 8:29 PM
lib/libc/tests/stdio/gets_s_test.c
31

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

33

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

38

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

51

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

65

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

cy marked 2 inline comments as done.Nov 26 2017, 7:02 AM
cy added inline comments.
lib/libc/tests/stdio/gets_s_test.c
31

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

38

That had crossed my mind.

65

Oops.

cy marked 6 inline comments as done.Feb 21 2018, 2:45 AM
cy updated this revision to Diff 39700.Feb 25 2018, 4:56 AM
cy marked an inline comment as done.

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

wblock added a subscriber: wblock.Mar 2 2018, 8:43 PM
wblock added inline comments.
lib/libc/stdio/fgets.3
97
112

As above:

.Fn gets_s ,
cy planned changes to this revision.Mar 7 2018, 3:58 AM
cy marked an inline comment as done.

I will upload fixes.

cy updated this revision to Diff 40025.Mar 7 2018, 3:59 AM

This addresses two punctuation issues wblock raised.

cy added a comment.Mar 7 2018, 4:02 AM
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 added a comment.Mar 12 2018, 7:16 PM

Are there any more comments or requested revisions?

cy added a comment.Mar 23 2018, 1:05 AM

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

ed accepted this revision.Mar 24 2018, 8:03 AM

Looks good to me. Thanks!

lib/libc/tests/stdio/gets_s_test.c
34

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

This revision was not accepted when it landed; it landed in state Needs Review.Apr 3 2018, 6:52 PM
This revision was automatically updated to reflect the committed changes.
emaste added inline comments.Apr 3 2018, 7:41 PM
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 added a comment.Apr 3 2018, 7:48 PM

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