<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 18, 2014 at 11:29 AM, Frederic Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">================<br>
Comment at: tools/dsymutil/BinaryHolder.cpp:59<br>
@@ +58,3 @@<br>
+  if (!Filename.startswith(CurArchiveName) ||<br>
+      Filename[CurArchiveName.size()] != '(')<br>
+    return make_error_code(errc::no_such_file_or_directory);<br>
----------------<br>
</span><span class="">samsonov wrote:<br>
> friss wrote:<br>
> > samsonov wrote:<br>
> > > You should check that Filename is long enough.<br>
> > If Filename startsWith CurArchiveName, then FileName[CurArchiveName.size()] is valid because these StringRefs come from std::strings that are null terminated.<br>
> Right, but if StringRef is a type of an argument in your public interface, it's better not to depend on the fact that these StringRef's will come from a particular source.<br>
</span>Makes sense, I'll add the check.<br></blockquote><div><br>I don't think you actually need the check - if Filename startsWith CurAchiveName, then Filename must be at least CurArchiveName.size() long, and std::string's operator[] is defined (in C++11) for the case where the index == size():<br><br><a href="http://en.cppreference.com/w/cpp/string/basic_string/operator_at">http://en.cppreference.com/w/cpp/string/basic_string/operator_at</a><br><br>You are guaranteed that if you do str[str.size()] you'll get '\0', which is fine for your pusrposes. If I'm understanding this all correctly... <br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: tools/dsymutil/BinaryHolder.cpp:78<br>
@@ +77,3 @@<br>
+BinaryHolder::MapArchiveAndGetMemberBuffer(StringRef Filename) {<br>
+  StringRef ArchiveFilename = Filename.substr(0, Filename.find('('));<br>
+<br>
----------------<br>
</span><span class="">samsonov wrote:<br>
> friss wrote:<br>
> > samsonov wrote:<br>
> > > Hm, don't you need to take the last opening parenthesis?<br>
> > Not here, we are extracting the archive name. When we extract the member name in GetArchiveMemberBuffer we drop the last character.<br>
> I mean last *opening* parenthesis. Can we have a Filename:<br>
>   my(small)archive.a(member.o)<br>
><br>
</span>Sorry for the confusion. Unfortunately, I think there's no nice way to handle this. The member could very well have an opening parenthesis in its name, making the last opening parenthesis the wrong one too. An option would be to try every parenthesis in the filename in turn, but I'd rather leave that out of this patch (not sure we ever want to handle it).<br>
<div class=""><div class="h5"><br>
<a href="http://reviews.llvm.org/D6690" target="_blank">http://reviews.llvm.org/D6690</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div></div></div>