Changed implementation of FileStorage#getFilePath to take in consider… · frostwire/frostwire-jlibtorrent@a932171

any chance of sending libtorrent a patch for that function?

arvid said he'd accept patches via github, through a branch on my fork (as long as I keep it up to date)

No, the problem is not in libtorrent, is in the swig generated automatic code

gubatron · 2014-11-04T21:10:26Z

should I prepare RC-build5 for windows after this, or is there more work in progress?

aldenml · 2014-11-04T21:11:36Z

Working in search, but in research mode

gubatron · 2014-11-04T21:17:49Z

I think search might take about a week to get it right and test it, so if this fixes the crash issue for sure, then I'll go ahead and make build 5.

aldenml · 2014-11-04T21:21:42Z

OK!

gubatron · 2014-11-04T21:30:14Z

just so I understand, what do you mean by "a copy not made"

       std::string file_storage::file_path(int index, std::string const& save_path) const
        {
                TORRENT_ASSERT_PRECOND(index >= 0 && index < int(m_files.size()));
                internal_file_entry const& fe = m_files[index];

                // -2 means this is an absolute path filename                                                                                                                                                                         
                if (fe.path_index == -2) return fe.filename();

                // -1 means no path                                                                                                                                                                                                   
                if (fe.path_index == -1) return combine_path(save_path, fe.filename());

                if (fe.no_root_dir)
                        return combine_path(save_path
                                , combine_path(m_paths[fe.path_index]
                , fe.filename()));

                return combine_path(save_path
                        , combine_path(m_name
                        , combine_path(m_paths[fe.path_index]
                        , fe.filename())));
        }

what is coming in save_path as a parameter that breaks this function?

gubatron · 2014-11-04T21:33:05Z

combine_path breaks? or breaks before even calling combine_path?

       std::string combine_path(std::string const& lhs, std::string const& rhs)
        {
                TORRENT_ASSERT(!is_complete(rhs));
                if (lhs.empty() || lhs == ".") return rhs;
                if (rhs.empty() || rhs == ".") return lhs;

#if defined(TORRENT_WINDOWS) || defined(TORRENT_OS2)
#define TORRENT_SEPARATOR "\\"
                bool need_sep = lhs[lhs.size()-1] != '\\' && lhs[lhs.size()-1] != '/';
#else
#define TORRENT_SEPARATOR "/"
                bool need_sep = lhs[lhs.size()-1] != '/';
#endif
                std::string ret;
                int target_size = lhs.size() + rhs.size() + 2;
                ret.resize(target_size);
                target_size = snprintf(&ret[0], target_size, "%s%s%s", lhs.c_str()
                        , (need_sep?TORRENT_SEPARATOR:""), rhs.c_str());
                ret.resize(target_size);
                return ret;
        }

aldenml · 2014-11-04T21:33:19Z

Look at this function Java_com_frostwire_jlibtorrent_swig_libtorrent_1jni_file_1storage_1file_1path_1_1SWIG_10 in https://raw.githubusercontent.com/frostwire/frostwire-jlibtorrent/master/swig/libtorrent_jni.cpp

The line const char *arg3_pstr = (const char *)jenv->GetStringUTFChars(jarg3, 0); does not pass any out-argument to determine if the array returned is a copy or not.

gubatron · 2014-11-04T21:36:38Z

GetStringUTFChars
const char * GetStringUTFChars(JNIEnv *env, jstring string,
jboolean *isCopy);

Returns a pointer to an array of bytes representing the string in modified UTF-8 encoding. This array is valid until it is released by ReleaseStringUTFChars().

If isCopy is not NULL, then *isCopy is set to JNI_TRUE if a copy is made; or it is set to JNI_FALSE if no copy is made.

gubatron · 2014-11-04T21:41:35Z

so if you had that out argument to determine wether the returned array was a copy or not, (wether you decided to use it or not) it wouldn't crash?

is it crashing because the method signature being used in the .cpp is wrong?

meaning, using the function like that won't magically assign an isCopy value anywhere, and when it tries to write wether it copied or not, it's basicaly writing to the wrong place in memory the resulting output and causing the crash?

So not a libtorrent issue, a bad JNI API usage issue?

gubatron · 2014-11-04T21:47:03Z

I see that this automatically generated file (by swig) makes use of GetStringUTFChars 216 times without explicitly passing the isCopy parameter.

Aren't we opened to more (possibly 215) crashes? (if I understood correctly the crash cause)

$ grep GetStringUTFChars libtorrent_jni.cpp | wc -l
     216

aldenml · 2014-11-04T21:53:22Z

It's not necessary in every case, only if you perform some memory work like in combine_path. In any case, I couldn't replicate the issue.

gubatron · 2014-11-04T21:58:06Z

oh wait, I think I read an old documentation as I see this is a function from jenv, and jenv is not being passed as a parameter.

Meaning

NOT:
GetStringUTFChars(jenv, jstring, jboolean)

We have
jenv->GetStringUTFChars(jstring, jboolean)

so if Swig is passing 0 for the jboolean output parameter... it's like if we're passing JNI_FALSE there
looking at JNI sources

Documentation reads...

If isCopy is not NULL, then *isCopy is set to JNI_TRUE if a copy is made; or it is set to JNI_FALSE if no copy is made.

is NULL == JNI_FALSE in JNI land?

If that were the case, then it's not using the output value... why is it crashing? i still don't know, but I see a lot of people complaining because of that function crashing everywhere.

aldenml · 2014-11-04T22:01:51Z

If you don't pass what isCopyargument, you will never know if the chart* returned is a copy or not. If you later use this char* in a constructor of std::string or any other sort of memory work, you don't know if the free should be called or not on this pointer.

gubatron · 2014-11-04T22:15:55Z

so you think the crash is due to it making a copy, yet still trying to release it?

jenv->ReleaseStringUTFChars(jarg3, arg3_pstr);

gubatron · 2014-11-04T22:16:26Z

(sorry I'm so slow @aldenml)

aldenml · 2014-11-04T22:28:58Z

(I know it's not easy :)

I think the crash could be because under some particular environment (hard to say when), the function GetStringUTFChars does not return a copy, then when you pass this pointer to the actual file_path(index, string) the pointer at some point could be elected to be released by the native code, but this pointer belongs to the jvm, since it was not a copy, then crash!

Again, pure theory.

-Original file line number
+Diff line change
@@ Expand Up / @@ -2,6 +2,8 @@ @@
     import com.frostwire.jlibtorrent.swig.file_storage;
+    import java.io.File;
     /**
      * The ``file_storage`` class represents a file list and the piece
      * size. Everything necessary to interpret a regular bittorrent storage
@@ Expand Down Expand Up / @@ -92,7 +94,10 @@ public Sha1Hash getHash(int index) { @@
          * @return
          */
         public String getFilePath(int index, String savePath) {
-            return fs.file_path(index, savePath);
+            // not calling the corresponding swig function because internally,
+            // the use of the function GetStringUTFChars does not consider the case of
+            // a copy not made
+            return savePath + File.separator + fs.file_path(index);
         }
         /**
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commit

There are no files selected for viewing

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

16 comments on commit `a932171`

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

Commit

There are no files selected for viewing

This comment has been minimized.

gubatron Nov 4, 2014

This comment has been minimized.

gubatron Nov 4, 2014

This comment has been minimized.

aldenml Nov 4, 2014

16 comments on commit a932171

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

aldenml commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

aldenml commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

aldenml commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

aldenml commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

aldenml commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

gubatron commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

aldenml commented on a932171 Nov 4, 2014

Choose a reason for hiding this comment

16 comments on commit `a932171`

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

gubatron commented on `a932171` Nov 4, 2014

aldenml commented on `a932171` Nov 4, 2014