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

bugfix for messageobj.c for windows vc++ #800

Closed
sonyps5201314 opened this issue Jan 30, 2019 · 3 comments
Closed

bugfix for messageobj.c for windows vc++ #800

sonyps5201314 opened this issue Jan 30, 2019 · 3 comments
Labels

Comments

@sonyps5201314
Copy link

 src/messageobj.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/messageobj.c b/src/messageobj.c
index a6a0bef..10c3c1c 100644
--- a/src/messageobj.c
+++ b/src/messageobj.c
@@ -643,6 +643,7 @@ static struct printfArg* BuildArgArray( TidyDocImpl *doc, ctmbstr fmt, va_list a
         else
         {
             strncpy(nas[cn].format, fmt + nas[cn].formatStart, nas[cn].formatLength);
+			nas[cn].format[nas[cn].formatLength] = 0;
         }
@geoffmcl
Copy link
Contributor

geoffmcl commented Feb 9, 2019

@sonyps5201314 thank you for the issue, through a suggested patch fix no less...

After testing, and some debugging, I agree, but the issue is more something like messageobj API can return a non null terminated string!, or something... more descriptive... but a bug... but you have found it...

Research

Yes, it does look like the MSVC docs strncpy, which documents, in part, If count is less than or equal to the length of strSource, a null character is not appended ...... AWK!

And in unix docs, like strncpy, says, in part If there is no null byte among the first n bytes of src ..., no null termination... AWK!

Yikes, unix and windows docs agree... sort of... ;=))

Test Case

Any clean html... any tidy version, probably since the messageobj first appeared...

Now, one of the most commonly seen outputs is Info: Document content looks like %s... in this case fmt + nas[cn].formatStart sets source as %s, length 2, being near end of the string...

And the count, nas[cn].formatLength, would also be 2...

Ipso facto, only %s would be copied to the destination, nas[cn].format... no null added ...

This dst (fixed buf len 21) can be seen in MSVC as %s!!!!..., where the ! character represents the debug fill done... D.AWK!

The Fix

Applying your patch, and adding a comment, suggest -

+            nas[cn].format[nas[cn].formatLength] = 0; /* Is. #800 - If count <= srcLen, no 0 added! */

Note, nas[cn].formatLength count has already been checked with if ( nas[cn].formatLength >= FORMAT_LENGTH ), so no possibility of buffer overrun...

While this copied message format is not presently later used in libTidy, so maybe this bug has remained quietly hidden...

However, its contents are available, to libTidy users, as a C-string, a tidy ctmbstr, through API tidyGetArgFormat, so must be null termainated...

Thank you for finding this... are you using this API in your project?

Will apply this patch, baring any negative comments, soonest... remind me if I am too long... thanks...

@geoffmcl geoffmcl added the Bug label Feb 9, 2019
@sonyps5201314
Copy link
Author

I am not use this API in my project,I debug tidy.exe with some test html find this problem.
I use tidy to fix web html to xhtml, then use tinyxml2 to get the information from the formated xhtml.

@geoffmcl
Copy link
Contributor

@sonyps5201314, eventually got around to pushing this small fix - years later ;=))

Again thanks for spotting the issue...

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