Bug 65955 - md_store_fs.c:524:72: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
Summary: md_store_fs.c:524:72: warning: ‘%s’ directive argument is null [-Wformat-over...
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_md (show other bugs)
Version: 2.5-HEAD
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk
Depends on:
Blocks:
 
Reported: 2022-03-15 07:51 UTC by David Binderman
Modified: 2022-03-18 17:50 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Binderman 2022-03-15 07:51:51 UTC
Source code is

       md_log_perror(MD_LOG_MARK, MD_LOG_ERR, rv, p, "mk_group_dir %d %s", group, name);

Culprit line is line 630:

   if (   MD_OK(mk_group_dir(&gdir, s_fs, group, NULL, p))

It looks wrong to me for the fourth parameter to be NULL.
Comment 1 Stefan Eissing 2022-03-15 08:59:20 UTC
The path of the directory to create (if it does not exist), is concatenated from the parameters (group, name, NULL). With name == NULL, this is the group directory itself.

In the md store, saving the JSON for a MDomain has the path:

md/staging/www.some.org/md.json

- line 630 makes sure md/staging exists
- line 631 makes sure md/staging/www.some.org exists

and subsequent lines then create the md.json file.

Logging in Apache something like "%s", NULL is allowed and will give the string "(null)".

With this in mind, does it still look wrong to you? If so, how?
Comment 2 David Binderman 2022-03-15 09:11:17 UTC
Ok, more detail: 

The compiler warns about the %s. This is the fourth parameter "name", of routine mk_group_dir.

This routine is marked static, so we know it is only called in source code
file md_store_fs.c.

$ fgrep -n mk_group_dir ./modules/md/md_store_fs.c
497:static apr_status_t mk_group_dir(const char **pdir, md_store_fs_t *s_fs, 
518:    md_log_perror(MD_LOG_MARK, MD_LOG_TRACE3, rv, p, "mk_group_dir %s perm set", *pdir);
524:        md_log_perror(MD_LOG_MARK, MD_LOG_ERR, rv, p, "mk_group_dir %d %s", group, name);
630:    if (   MD_OK(mk_group_dir(&gdir, s_fs, group, NULL, p)) 
631:        && MD_OK(mk_group_dir(&dir, s_fs, group, name, p))

Line 630 is the smoking gun. QED.
Comment 3 Stefan Eissing 2022-03-16 08:29:13 UTC
Well, there is nothing "smoking" here. The compiler warns since printf formatting is not defined for NULL, but for Apache logging, it is.

But eliminating compiler warnings is good, so I made the change in r1898962. 

Will that work for you?
Comment 4 David Binderman 2022-03-16 09:05:38 UTC
>Will that work for you?

Mostly yes. Thanks.

Minor nit: it is IMHO poor style to nest ternary operators.

I wouldn't bother fixing it again, but you might want to
remember this for the future.