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

Maybe a problem with some vsnprintf implementations? #655

Closed
geoffmcl opened this issue Dec 5, 2017 · 2 comments
Closed

Maybe a problem with some vsnprintf implementations? #655

geoffmcl opened this issue Dec 5, 2017 · 2 comments
Labels
Milestone

Comments

@geoffmcl
Copy link
Contributor

geoffmcl commented Dec 5, 2017

In all my linux installations the option --mute-id yes does not give the correct message output. This was in Ubuntu 14.04 64-bit, 16.04 32-bit, and Raspbian 32-bit...

It works fine in Windows 10 64-bit, and XP 32-bit...

The problem appears to be the use of the output buffer as one of the inputs in the va_list... the stackoverflow post does indicate this may not be safe in all implementations... and found other similar indications...

If have prepared a small test_vsnprintf test app which should demonstrate the problem, if there is one...

Since I initially thought this might be a regression test problem for case 629, I filed a report there, Issue 26, and that included a patch, now repeated here, which fixed it for me -

diff --git a/src/messageobj.c b/src/messageobj.c
index 6f76152..7b9b415 100644
--- a/src/messageobj.c
+++ b/src/messageobj.c
@@ -158,8 +158,14 @@ static TidyMessageImpl *tidyMessageCreateInitV( TidyDocImpl *doc,
 
     if ( ( cfgBool(doc, TidyMuteShow) == yes ) && level <= TidyFatal )
     {
-        TY_(tmbsnprintf)(result->messageOutputDefault, sizeMessageBuf, "%s (%s)", result->messageOutputDefault, TY_(tidyErrorCodeAsKey)(code) );
-        TY_(tmbsnprintf)(result->messageOutput, sizeMessageBuf, "%s (%s)", result->messageOutput, TY_(tidyErrorCodeAsKey)(code) );
+        ctmbstr pc = TY_(tidyErrorCodeAsKey)(code);
+        i = TY_(tmbstrlen)(result->messageOutputDefault);
+        if (i < sizeMessageBuf)
+            TY_(tmbsnprintf)(result->messageOutputDefault+i, sizeMessageBuf-i, " (%s)", pc );
+        i = TY_(tmbstrlen)(result->messageOutput);
+        if (i < sizeMessageBuf)
+            TY_(tmbsnprintf)(result->messageOutput+i, sizeMessageBuf-i, " (%s)", pc );
+        i = 0;
     }
 
     result->allowMessage = yes;

Since this option was implemented by @balthisar, who also added case 629 at the same time, I have been waiting for his comments, but seems he is busy with RL at the moment...

It is not paticularly urgent, since as far as I can see it only effects the new option --muted-id yes, but would be appreciated if meantime others could test, apply the patch, and report... thanks...

@balthisar
Copy link
Member

@geoffmcl, sorry about the RL conflicts! It hurts more that I see the issues come in the inbox, but don't have an opportunity to provide feedback. This patch looks good. The guards look good, although it's probably not needed. I know starting in C11 a zero size in standard snprintf is okay, but I can't find any references in previous versions, to leave it in.

I'm still a bit overcommitted until this weekend. Feel free to merge this patch and bump the version at your convenience.

@geoffmcl
Copy link
Contributor Author

Created PR #662 for testing and review... thanks...

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

No branches or pull requests

2 participants