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

sanitize log entries before they enter circular buffer #959

Merged
merged 5 commits into from Jan 19, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Jan 18, 2017

Here's a fix for #939 that simply strips the eighth bit from all characters in logged messages.

I added a test that failed before and works with the fix.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 18, 2017

Current coverage is 75.90% (diff: 84.61%)

Merging #959 into master will increase coverage by 0.01%

@@             master       #959   diff @@
==========================================
  Files           152        152          
  Lines         25923      25937    +14   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19673      19688    +15   
+ Misses         6250       6249     -1   
  Partials          0          0          
Diff Coverage File Path
•••••• 66% src/common/libflux/flog.c
•••••••• 80% src/cmd/flux-logger.c
•••••••• 88% src/common/libutil/stdlog.c

Powered by Codecov. Last update 7332a0a...d04c43b

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage increased (+0.009%) to 76.193% when pulling 7ba603c on garlick:stdlog_Fix into e2528d1 on flux-framework:master.

garlick added some commits Jan 18, 2017

libutil/stdlog: add varargs encoding functions
Problem: the fixed message buffer encoding function for
RFC 5424 log data is awkward to use from the varargs
Flux logging function, its primary user.

Add encoding interface that accepts varargs message,
and refactor fixed message buffer function so that unit
tests will exercise the new code as well.
libflux/flog: use varargs stdlog_vencodef()
Problem: flux_vlog() writes log message directly to
log buffer, bypassing stdlog_encode() which is the logical
place to do any checking/conversion on the message.

Call the new stdlog_vencodef() function so we can add
checking/conversion there to ensure content is compatible
with RFC 5424 and our log.dmesg RPC.
cmd/flux-logger: optionally take message from stdin
Instead of issuing a fatal error if message is missing
from the command line, fall back to reading the message
from stdin like logger(1).

This behavior will be useful for writing a test that
tries to log "illegal" message content.
libutil/stdlog: convert all messages to ASCII
Problem: if a log message includes the right non-ASCII
character, a log message can enter the circular buffer
which might violate RFC 5424 and/or cannot be re-encoded
as a UTF-8 JSON string by for dmesg output.

RFC 5424 requires message content to follow this ABNF:

MSG             = MSG-ANY / MSG-UTF8
MSG-ANY         = *OCTET ; not starting with BOM
MSG-UTF8        = BOM UTF-8-STRING
BOM             = %xEF.BB.BF

Rather than allow all RFC 5424 MSG content, this patch simply
converts all messages to ASCII by clearing the 8th bit of every
byte, which fits both MSG-ANY and JSON's string requirements.
The caveat is that non-ASCII log data will possibly be a bit
mangled.  We can revisit this if we get serious about
internationalizing error messages, for example.

Fixes #939

@garlick garlick force-pushed the garlick:stdlog_Fix branch from 7ba603c to d04c43b Jan 19, 2017

test/dmesg: try to log some non-ASCII chars
Add a test that tries to insert non-ASCII characters into
the log and elicit an EPROTO when they are read back out
with flux-dmesg.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jan 19, 2017

Rebased and reworked commit messages to be more clear.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jan 19, 2017

This seems good to me, after travis passes is this ready to merge?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 19, 2017

Coverage Status

Coverage increased (+0.02%) to 76.179% when pulling d04c43b on garlick:stdlog_Fix into 7332a0a on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jan 19, 2017

OK, thanks.

@grondo grondo merged commit 1284add into flux-framework:master Jan 19, 2017

4 checks passed

codecov/patch 84.61% of diff hit (target 75.89%)
Details
codecov/project 75.90% (+0.01%) compared to 7332a0a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 76.179%
Details

@garlick garlick deleted the garlick:stdlog_Fix branch Jan 19, 2017

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.