Mis-used string pointers in opendkim.c and vbr.c#244
Open
jcastle-gh wants to merge 1 commit intotrusteddomainproject:developfrom
Open
Mis-used string pointers in opendkim.c and vbr.c#244jcastle-gh wants to merge 1 commit intotrusteddomainproject:developfrom
jcastle-gh wants to merge 1 commit intotrusteddomainproject:developfrom
Conversation
Enhanced warnings in Debian and Fedora build logs expose some string addressing problems in vbr.s and opendkim.c. 1. In https://bugs.debian.org/1075339 which is about a different problem, the build log attached to the bug shows a warning from -DFORTIFY_SOURCE: vbr.c: In function 'vbr_query' vbr.c:1069:43: warning: "unable to start resolver for.." directive output truncated writing 30 bytes into a region of size 8 [-Wformat-truncation=] 1069 | "unable to start resolver for '%s'", | ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~ vbr.c:1081:43: warning: "unable to start query for '" directive output truncated writing 27 bytes into a region of size 8 [-Wformat-truncation=] 1081 | "unable to start query for '%s'", | ~~~~~~~~^~~~~~~~~~~~~~~~~~~ The size of the buffer pointer is mistakenly used instead of the size of the buffer itself, resulting in a truncated error message. 2.Looking at the Fedora build log for similar issues shows one in opendkim.c. https://kojipkgs.fedoraproject.org//packages/opendkim/2.11.0/0.42.fc43/data/logs/x86_64/build.log opendkim.c: In function 'dkimf_add_signrequest': opendkim.c:5023:38: warning: the comparison will always evaluate as 'false' for the address of 'mctx_domain' will never be NULL [-Waddress] 5023 | dfc->mctx_domain == NULL) | ^~ opendkim.c:523:25: note: 'mctx_domain' declared here 523 | unsigned char mctx_domain[DKIM_MAXHOSTNAMELEN + 1]; | ^~~~~~~~~~~ The code is checking if mctx_domain is an empty string. 'mctx_domain' is the string buffer itself, not a malloced pointer to it, so the code should be looking for "mctx_domain[0] == '\0'". Later code uses the string assuming it's not empty, so it seems a good idea to fix it regardless.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enhanced warnings in Debian and Fedora build logs expose some string addressing problems in vbr.c and opendkim.c.
In two snprintf() calls the size of the buffer pointer is mistakenly used instead of the size of the buffer itself, resulting in a truncated error message. There is already a static function vbr_error() which can safely replace snprintf here.
https://kojipkgs.fedoraproject.org//packages/opendkim/2.11.0/0.42.fc43/data/logs/x86_64/build.log
The code is trying to return an error if mctx_domain is an empty string. 'mctx_domain' is the string buffer itself, not a malloced pointer to it, so the code should be looking for "mctx_domain[0] == '\0'". This bug means later code uses the possibly empty string assuming it's not empty, so it should be fixed even without a failing test case.