Skip to content

Don't segfault when product name from BIOS is empty#15929

Merged
nicolasnoble merged 2 commits intogrpc:masterfrom
Torgen:empty_product_name_segfault
Jul 21, 2018
Merged

Don't segfault when product name from BIOS is empty#15929
nicolasnoble merged 2 commits intogrpc:masterfrom
Torgen:empty_product_name_segfault

Conversation

@Torgen
Copy link
Copy Markdown
Contributor

@Torgen Torgen commented Jul 3, 2018 •
edited
Loading

Somehow some Dell servers we're trying to run a gRPC client on have an empty product name in the BIOS. When gRPC tries to create default credentials, it checks whether it's running on GCE by strcmp()ing the contents of /sys/class/dmi/id/product_name to some magic strings. When it reads that file, it gets only a newline; in trim() it skips over the newline in both directions, and since end < start it returns nullptr. This causes a segfault in the strcmp() call. Since a machine without a product name clearly isn't GCE, change it to return false instead.

@Torgen
Don't segfault when product name from BIOS is empty
477a1ae 
Somehow some Dell servers we're trying to run a gRPC client on have an empty product name in the BIOS. When gRPC tries to creadte default credentials, it checks whether it's running on GCE by strcmp()ing the contents of /sys/class/dmi/id/product_name to some magic strings. When it reads that file, it gets only a newline; in trim() it skips over the newline in both directions, and since end < start it returns nullptr. This causes a segfault in the strcmp() call. Since a machine without a product name clearly isn't GCE, change it to return false instead.
@vjpai vjpai requested a review from yihuazhang July 3, 2018 20:21
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 6, 2018 

****************************************************************

libgrpc.so

     VM SIZE                                                                             FILE SIZE
 ++++++++++++++ GROWING                                                               ++++++++++++++
  +0.0%     +16 [None]                                                                    +72  +0.0%
  +8.5%     +16 src/core/lib/security/credentials/alts/check_gcp_environment_linux.cc     +16  +8.5%
       +45%     +29 grpc_core::internal::check_bios_data                                      +29   +45%

  +0.0%     +32 TOTAL                                                                     +88  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

yihuazhang
yihuazhang approved these changes Jul 6, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@yihuazhang yihuazhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure the PR passes all tests before merging it.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 6, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 6, 2018 

[microbenchmarks] No significant performance differences

@Torgen
Copy link
Copy Markdown
Contributor Author

Torgen commented Jul 6, 2018

PTAL. Clang format diffs were the only failing test in the sanity check.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 20, 2018 

****************************************************************

libgrpc.so

     VM SIZE                                                                             FILE SIZE
 ++++++++++++++ GROWING                                                               ++++++++++++++
  +0.0%     +16 [None]                                                                    +64  +0.0%
  +8.5%     +16 src/core/lib/security/credentials/alts/check_gcp_environment_linux.cc     +16  +8.5%
       +45%     +29 grpc_core::internal::check_bios_data                                      +29   +45%

  +0.0%     +32 TOTAL                                                                     +80  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 20, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 20, 2018 

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,975,261      Total (=)      1,975,261

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,769,652      Total (>)     10,769,638

 No significant differences in binary sizes

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Jul 20, 2018 

[microbenchmarks] No significant performance differences

@nicolasnoble nicolasnoble merged commit 2cfc216 into grpc:master Jul 21, 2018
@Torgen Torgen deleted the empty_product_name_segfault branch July 21, 2018 07:37
@coryan
Copy link
Copy Markdown
Contributor

coryan commented Aug 20, 2018

@srini100 any plans to backport this PR to v1.14.x? We are running into crashes with v1.14.1 and they seem to be related to this problem.

@srini100
Copy link
Copy Markdown
Contributor

srini100 commented Aug 20, 2018

@coryan, will do an RC hopefully by tomorrow and release 1.14.2 patch after a few days. Have you verified the fix on master or using nightly builds from https://packages.grpc.io/ ?

srini100 pushed a commit to srini100/grpc that referenced this pull request Aug 21, 2018
@nicolasnoble @srini100
Merge pull request grpc#15929 from Torgen/empty_product_name_segfault
aaa3162 
Don't segfault when product name from BIOS is empty
@srini100 srini100 mentioned this pull request Aug 21, 2018
Merged
srini100 added a commit that referenced this pull request Aug 21, 2018
@srini100
Merge pull request #16405 from srini100/v1.14.x
08bf39a 
Backport PRs #15929 and #16296 from master
@srini100
Copy link
Copy Markdown
Contributor

srini100 commented Aug 22, 2018

@coryan, 1.14.2 RC is now available.

@coryan
Copy link
Copy Markdown
Contributor

coryan commented Aug 23, 2018

@srini100, good news, it works for us. Thanks for the quick turnaround.

@jtattermusch jtattermusch mentioned this pull request Oct 19, 2018
Merged
@nicolasalt
Copy link
Copy Markdown

nicolasalt commented Nov 2, 2018

Would it be possible to merge this fix to 1.13 which is still used by TensorFlow?

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/workspace.bzl

@nicolasnoble
Copy link
Copy Markdown
Contributor

nicolasnoble commented Nov 3, 2018

We'll work with tensorflow on getting them to a newer version of grpc.

@nicolasalt
Copy link
Copy Markdown

nicolasalt commented Nov 3, 2018

Thanks Nicolas!

@lock lock bot locked as resolved and limited conversation to collaborators Feb 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@yihuazhang yihuazhang yihuazhang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@Torgen @grpc-testing @coryan @srini100 @nicolasalt @nicolasnoble @yihuazhang @kokoro-team