Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed null ptr errors in print statements #635

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jayjaybillings
Copy link

I was compiling from source on Fedora 32 using GCC 10.1.1-1 and encountered numerous compilation errors. GCC 9 and 10 are rather picky about calling printf with arguments that can have NULL values.

To fix these errors I adjusted the vcos_logging header file and fixed two separate places in the file system and threading utilities.

Please let me know if I need to adjust anything. Happy to help get this in shape to push to master.

@6by9
Copy link
Contributor

6by9 commented Jun 15, 2020

Thanks for the patch

Personally I hate if( const != variable), but also these can all be condensed down to if (var)

I'm also not quite clear what the logic is with the gcc change. printf("%s", NULL) always used to print "(null)", so throwing an error is really a backwards step. We're now not logging anything under these conditions, so perhaps they really ought to be eg

const char* null_str = "(null)";
DEBUG_MINOR( "vc_hostfs_totalspace for '%s' returning %" PRId64 "", path ? path : null_str, ret );

which is less readable, and you'll end up with loads of copies of a string "(null)" kicking around.

I wonder what others think.

@popcornmix
Copy link
Contributor

I agree printing "(null)" is more useful for debugging that printing nothing so there is some cost to fixing the issues.

Do you have some examples of gcc error messages? I wonder if it's better (and possible) to handle them from caller code - I suspect if we have got to a point where we have a null pointer and are printing it out we may be potentially dereferencing it through means other than printf.

@tombsar
Copy link

tombsar commented Jun 15, 2020

Surely you should either make vcos_vlog_default_impl handle a NULL cat pointer gracefully, OR update the _VCOS_LOG_X macros to avoid ever passing one, but not both?

Note that vcos_vlog_default_impl can still crash after this patch due to the unconditional pointer dereference on line 301.

@6by9
Copy link
Contributor

6by9 commented Jun 15, 2020

I'm expecting this should fix #631 which lists the error messages.

AFAIK vcos_vlog_default_impl will never be called with cat = NULL, but you can't tell the compiler that, and it complains. You could assert, but that only does anything on debug builds.

It makes no sense whatsoever as the code would segfault first at the deref of cat

         if (cat->flags.want_prefix) <<<<<<
            fprintf( log_fhandle, "%s: ", cat->name );

but that's not what the compiler is complaining of.
Sorry, my comment is twaddle. and the patch doesn't fix this.
cat->name could be null, and that is what the compiler is really complaining about, it doesn't do any checking of cat itself.

Line 301 is under a #idef ANDROID which is never defined on the Pi.

@tombsar
Copy link

tombsar commented Jun 15, 2020

AFAIK vcos_vlog_default_impl will never be called with cat = NULL

Right, so I don't think this patch actually helps the printf problem.

It looks to me as though the proper check would be if (cat->flags.want_prefix && cat->name). Hopefully that's straightforward enough for gcc to understand, and fix the over-zealous compile errors.

Line 301 is under a #idef ANDROID which is never defined on the Pi.

I saw that, but thought that since the code remains in the function, it should be kept up to date with its surroundings. Just curious, but is there any reason these parts of the code are kept in the repository if they are for sure never compiled?

@jayjaybillings
Copy link
Author

jayjaybillings commented Jun 15, 2020

I just pushed changes to fix the if statements. Now they check path and cat->name directly.

@jayjaybillings
Copy link
Author

jayjaybillings commented Jun 15, 2020

@tombsar I was just reading your comment more closely. FWIW I agree. I don't thinks this fixes the broader set of printf problems for GCC 10, but it at least gets it compiling on Fedora 32.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants