Skip to content

Commit

Permalink
Address first issue
Browse files Browse the repository at this point in the history
  • Loading branch information
@ohler55
ohler55 committed Jan 27, 2019
1 parent 7ff0673 commit e15f516
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions src/perfer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,10 @@ build_req(Perfer p) {
for (Header h = p->headers; NULL != h; h = h->next) {
size += strlen(h->line) + 2;
}
p->req_body = (char*)malloc(size + 1);
if (NULL == (p->req_body = (char*)malloc(size + 1))) {
printf("*-*-* Out of memory.\n");
exit(-1);
}
p->req_len = size;
end = p->req_body;
if (p->keep_alive) {
Expand All @@ -129,7 +132,10 @@ perfer_init(Perfer p, int argc, const char **argv) {
int cnt;
long num = 0;

pthread_mutex_init(&p->print_mutex, 0);
if (0 != pthread_mutex_init(&p->print_mutex, 0)) {
printf("%s\n", strerror(errno));
return -1;
}
argv++;
argc--;
for (; 0 < argc; argc -= cnt, argv += cnt) {
Expand Down

16 comments on commit e15f516

@elfring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, perror could be used. Not a big deal either way though.

Nicer as in say "Address issue #1" instead of first issue?

@elfring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The function “perror” has got the return type “void” and the output channel “stderr” is selected instead.
  • Would a commit subject like “Addition of exception handling for two functions” have been more appropriate (because a few update candidates were still left over in the source code)?

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, writing to stderr in this case is not really what I want. It is often the case this tool is used in a script to gather results. It will take more effort on the user's part to include both strerr and stdout in the same stream or file. Better to leave it the way it is I think. I'll keep it in mind for the next time though. Thanks.

Other candidates? Can you describe the other candidates? Something like "malloc return values should be checked" or something like that if thats what you are referring to.

@elfring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should error output redirection be applicable?

Function calls for further considerations:

  • fclose
  • fseek
  • ftell
  • malloc
  • printf
  • pthread_mutex_lock
  • pthread_mutex_unlock

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for that list. I'll go through the code later today.

Output redirection would be applicable if using perror since it outputs on stderr. Someone running in a script that did not capture strerr would miss the reason for missing output in the output stream.

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed addition checked. I did not put any code to check returned on the mutex lock and unlock for reasons explained previously. I do not thing putting a check on every printf is necessary. If it fails it will be pretty obvious and I'd expect the app to crash at that point anyway. Remember this is a utility for benchmarking.

@elfring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hardly thing not checking the return values of a printf statement in a benchmark utility is a security issue. If it is too insecure then anyone that finds the utility too insecure for benchmarking does not have to use it.

@elfring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • How much can you trust data processing results from such a program if it is generally insecure because of return value ignorance?
  • Do you find any more details from the linked information sources relevant for your software?

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you image there could be any kind of security issue with printf? Think about the use cases for a benchmark utility. A failed printf will not have any impact at all other than not showing results.

What other linked information sources are you referring to?

@elfring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I would prefer a program which will immediately report an error if the desired file output failed.
  • There are function calls remaining where their return values did not get the needed software development attention so far.
  • I suggest to take another look at the article “CWE-252: Unchecked Return Value”.

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a printf fails, how would you report the error? If printf is broken there is no way to report the error. The only other action would be to just exit silently which is not very helpful. In any case complaining about the use of printf in a benchmarking utility is a bit extreme.

What other functions are you referring to?

@elfring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • How do you think about to indicate a file output failure by a corresponding exit status for your program?
  • It seems that you are still reluctant to handle the return value relevance from the mentioned function list completely.

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am reluctant to add checks for printf, pthread_mutex_lock and pthread_mutex_unlock for reasons I have already explained. I see no value in checking return values for those functions as it only makes the code harder to read and does not protect against any real world issues.

@elfring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Your reluctance makes also this program implementation insecure so far.
  • I guess that more time (or other contributors) would be needed to adjust a software development view a bit more here.

Please sign in to comment.